Skip to content
This repository has been archived by the owner on Jun 27, 2022. It is now read-only.

Rework hw-app-eth to split out "ledger services" from the app logic #747

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

gre
Copy link
Contributor

@gre gre commented Jan 3, 2022

With this PR,

eth.signTransaction(path, txHex)

becomes deprecated in favor of

eth.signTransaction(path, txHex, resolution)

where resolution is an object with following shape:

{
  erc20Tokens: Array<string>;
  nfts: Array<string>;
  externalPlugin: Array<{ payload: string; signature: string }>;
  plugin: Array<string>;
}

This object contains all the hex serialized data the device may need to clear sign information on device screen.

Hopefully we also provide a default "service" to resolve this data, example:

import ledgerService from "@ledgerhq/hw-app-eth/lib/services/ledger"
async function main() {
  ...
  const resolution = await ledgerService.resolveTransaction(txHex)
    .catch(() => null); // <-- up to you if you want a fallback to blind sign. (which was previous behavior)
   const result = eth.signTransaction("44'/60'/0'/0/0", txHex, resolution);
}

This allows us to decouple the transaction resolution from the device signature logic itself in order for you to have better control and for more usecases

  • an alternative service (rather than Ledger's) could be used.
  • we plan to make even more things dynamic, typically the way we load and resolve ERC20 and plugins are too monolithic. This rework pave the path to allow this even more easily.
  • you may want to load the resolve data way ahead in your "validation process" and typically inform the user if an error occurs before it reach the device phase.
  • you may want a finer control weither you allow blind sign or not. Typically, you may or may not use .catch(() => null) in the snippet above

@gre gre requested a review from a team as a code owner January 3, 2022 17:03
@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #747 (e052782) into master (d40c77c) will decrease coverage by 0.89%.
The diff coverage is 87.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #747      +/-   ##
==========================================
- Coverage   45.00%   44.10%   -0.90%     
==========================================
  Files          81       77       -4     
  Lines        4911     4757     -154     
  Branches      811      790      -21     
==========================================
- Hits         2210     2098     -112     
+ Misses       2684     2642      -42     
  Partials       17       17              
Impacted Files Coverage Δ
packages/hw-app-eth/src/Eth.ts 78.86% <85.93%> (-1.92%) ⬇️
packages/hw-app-eth/src/utils.ts 91.17% <91.17%> (+37.68%) ⬆️
...ckages/hw-app-eth/src/services/ledger/contracts.ts
packages/hw-app-eth/src/services/ledger/erc20.ts
packages/hw-app-eth/src/services/ledger/nfts.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d40c77c...e052782. Read the comment docs.

Comment on lines +286 to +287
if (chainId.times(2).plus(35).plus(1).isGreaterThan(255)) {
const oneByteChainId = (chainIdTruncated * 2 + 35) % 256;
Copy link

@ghost ghost Jan 4, 2022

Choose a reason for hiding this comment

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

Fully trusting you on the voodoo here and below 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only moved code on this one.

@ghost ghost self-requested a review January 4, 2022 13:38
@ghost
Copy link

ghost commented Jan 4, 2022

No added test coverage for the new signature?

@gre gre merged commit ee71b98 into master Jan 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant