Update gh-pages to receive chainId, update Ledger library to 6.5.0 #95

darkwing · 2021-08-16T21:57:31Z

It's been requested that we update to 6.5.0 to fix Ledger issues with Palm. This PR:

Updates to 6.5.0 of the core Ledger library for the gh-pages branch
~~Receives the chainId from the eth-ledger-bridge-keyring package, since this version has a breaking change that sees byContractAddress become byContractAddressAndChainId.~~
removes the call to provideERC20TokenInformation as it is no longer needed, after v5.52.0

edited by Dan M on Aug 26

ledger-bridge.js

Gudahtt · 2021-08-17T16:53:27Z

ledger-bridge.js

-            if (to) {
-                const isKnownERC20Token = byContractAddress(to)
+            if (to && chainId) {
+                const isKnownERC20Token = byContractAddressAndChainId(to, chainId)
                if (isKnownERC20Token) await this.app.provideERC20TokenInformation(isKnownERC20Token)


Apparently we don't need to call this anymore, as of v5.52.0? https://github.com/LedgerHQ/ledgerjs/releases/tag/v5.52.0
I'm not 100% sure though.

I can confirm that we do not need this any more. The PR where this was fixed was:

LedgerHQ/ledgerjs#587

The provideERC20TokenInformation method had the description:

This commands provides a trusted description of an ERC 20 token

to associate a contract address with a ticker and number of decimals.

It shall be run immediately before performing a transaction involving a contract

calling this contract address to display the proper token information to the user if necessary.

The PR made a change so that the method is run before every transaction: https://github.com/LedgerHQ/ledgerjs/pull/587/files#diff-2a96855bc41e2457f03a382c30057bb9257533493bad6acba9730305db50dd60R252-R279

Meanwhile, the need to get the chainId is also covered by that library internally. It gets the chainId from the transaction data: https://github.com/LedgerHQ/ledgerjs/pull/647/files#diff-d69d98013c437f5ecea9591706f3c1319cfa49c1ec50130f8a6346291517ba3dL229

Finally, I tested ERC20 token sends with ledger + metamask + this current branch of github pages, and it works the same as prod today, with the token symbol displaying on the device.

ledger-bridge.js

danjm · 2021-08-18T02:10:45Z

So no changes to ledger-bridge.js are needed?

darkwing · 2021-08-18T02:12:29Z

Right

darkwing · 2021-08-18T20:12:14Z

Also a dependency: https://github.com/MetaMask/eth-ledger-bridge-keyring/pull/93/files

danjm

I did not review the diff of the package-lock.json files and bundle.js files

I did verify that the same diff (in terms of line count) is produced when I build locally

I did review the ledgerjs changelog to see that this change is safe

danjm

this needs changes actually

danjm

We should get another review because I ended up authoring a chunk of this PR, but I'm removing my requested change

Gudahtt

It looks like the lockfile and the bundle require updating. When I checkout this branch and run npm install, the package-lock.json file changes. Then when I run npm run build, the bundle changes.

danjm · 2021-08-26T10:47:01Z

@Gudahtt Interesting. I run:
rm -rf node_modules
npm install
npm run build

There are no changes in git.

What versions of npm and node are you on?

Gudahtt · 2021-08-26T11:47:46Z

package-lock.json

@@ -1,13 +1,11349 @@
 {
  "name": "gupltest",
  "version": "0.0.0",
-  "lockfileVersion": 1,
+  "lockfileVersion": 2,


Ah, this is the issue. You must be using npm v7 - previously this repo was using v6 (which is what I was using).

v7 is fine but we should bump the npm version in the engines field at least, so this mistake can be prevented.

Gudahtt · 2021-08-26T17:22:54Z

The lockfile is still dirty, because the new engines field gets copied into the lockfile 😅

Gudahtt · 2021-08-26T17:25:04Z

ledger-bridge.js

@@ -143,10 +142,6 @@ export default class LedgerBridge {
    async signTransaction (replyAction, hdPath, tx, to) {


The to parameter is now unused

Removed in 231bd5d

Gudahtt

LGTM!

… after PR #95

… signTransaction (#98) * Safe property access / allow transaction with undefined to fields, in signTransaction * Revert "Safe property access / allow transaction with undefined to fields, in signTransaction" This reverts commit 3c148ba. * Remove passage of to param to ledger bridge, which will not be needed after PR #95

pscott · 2021-09-09T15:57:50Z

Thanks for your work! I think #90 can be closed now!

However I would like to point out that Ledgerjs recently released v6.6.0. This release fixes an important bug that prevented ERC20 token information to get passed to the device when using EIP1559 typed transactions (type 2). This means that currently, users sending EIP1559 transactions will be blind-signing rather than seing exactly which token is being transfered.

Bumping this version does not introduce any breaking changes, and would benefit the whole ecosystem, hence this message!

Thank you!

… signTransaction (MetaMask#98) * Safe property access / allow transaction with undefined to fields, in signTransaction * Revert "Safe property access / allow transaction with undefined to fields, in signTransaction" This reverts commit 3c148ba. * Remove passage of to param to ledger bridge, which will not be needed after PR MetaMask#95

…etaMask#95) * Update gh-pages to receive chainId, update Ledger library to 6.4.1 * Add safety so that old and new lookup work properly * Address feedback: check for chainId * Pin to specific version to avoid semver issues * v6.5.0 bycontractaddress removal * Removed unused import * Specificy npm v7.6.1 in engines property of package.json * Update lockfile * Remove unused parameter from signTransaction in ledger-bridge Co-authored-by: Dan Miller <[email protected]> Co-authored-by: Mark Stacey <[email protected]>

darkwing added 2 commits August 16, 2021 16:54

Update gh-pages to receive chainId, update Ledger library to 6.4.1

16dc9c3

Add safety so that old and new lookup work properly

c52a836

darkwing commented Aug 16, 2021

View reviewed changes

ledger-bridge.js Outdated Show resolved Hide resolved

This was referenced Aug 16, 2021

Update Ledger lib to 6.3.0 to support EIP-1559 #91

Closed

EIP-1559 - Provide support for Ledger MetaMask/metamask-extension#11786

Closed

Address feedback: check for chainId

5903a8a

Gudahtt reviewed Aug 17, 2021

View reviewed changes

ledger-bridge.js Show resolved Hide resolved

Pin to specific version to avoid semver issues

28c98a6

danjm approved these changes Aug 18, 2021

View reviewed changes

danjm requested changes Aug 25, 2021

View reviewed changes

danjm mentioned this pull request Aug 25, 2021

Fix 1559 #97

Merged

danjm added 2 commits August 25, 2021 15:46

v6.5.0 bycontractaddress removal

12f36c5

Removed unused import

6f6b7cd

danjm approved these changes Aug 25, 2021

View reviewed changes

Gudahtt requested changes Aug 26, 2021

View reviewed changes

Gudahtt reviewed Aug 26, 2021

View reviewed changes

Specificy npm v7.6.1 in engines property of package.json

9ca46a3

danjm changed the title ~~Update gh-pages to receive chainId, update Ledger library to 6.4.1~~ Update gh-pages to receive chainId, update Ledger library to 6.5.0 Aug 26, 2021

Update lockfile

190893e

Gudahtt reviewed Aug 26, 2021

View reviewed changes

Remove unused parameter from signTransaction in ledger-bridge

231bd5d

Gudahtt approved these changes Aug 26, 2021

View reviewed changes

This was referenced Aug 27, 2021

1559 ledger MetaMask/metamask-extension#11951

Merged

Safe property access / allow transaction with undefined to fields, in signTransaction #98

Merged

danjm added a commit that referenced this pull request Aug 27, 2021

Remove passage of to param to ledger bridge, which will not be needed…

8f2195c

… after PR #95

ryanml approved these changes Aug 27, 2021

View reviewed changes

adonesky1 merged commit 74a0747 into MetaMask:gh-pages Aug 30, 2021

adonesky1 mentioned this pull request Aug 31, 2021

v0.7.0 #100

Merged

pscott mentioned this pull request Sep 17, 2021

Please upgrade hw-app-eth to make it support EIP1559 and EIP2930 #90

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gh-pages to receive chainId, update Ledger library to 6.5.0 #95

Update gh-pages to receive chainId, update Ledger library to 6.5.0 #95

darkwing commented Aug 16, 2021 •

edited by danjm

Loading

Gudahtt Aug 17, 2021

danjm Aug 25, 2021

danjm commented Aug 18, 2021

darkwing commented Aug 18, 2021

darkwing commented Aug 18, 2021

danjm left a comment

danjm left a comment

danjm left a comment

Gudahtt left a comment

danjm commented Aug 26, 2021

Gudahtt Aug 26, 2021

Gudahtt commented Aug 26, 2021

Gudahtt Aug 26, 2021

danjm Aug 26, 2021

Gudahtt left a comment

pscott commented Sep 9, 2021 •

edited

Loading

		@@ -143,10 +142,6 @@ export default class LedgerBridge {
		async signTransaction (replyAction, hdPath, tx, to) {

Update gh-pages to receive chainId, update Ledger library to 6.5.0 #95

Update gh-pages to receive chainId, update Ledger library to 6.5.0 #95

Conversation

darkwing commented Aug 16, 2021 • edited by danjm Loading

Gudahtt Aug 17, 2021

Choose a reason for hiding this comment

danjm Aug 25, 2021

Choose a reason for hiding this comment

danjm commented Aug 18, 2021

darkwing commented Aug 18, 2021

darkwing commented Aug 18, 2021

danjm left a comment

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

danjm commented Aug 26, 2021

Gudahtt Aug 26, 2021

Choose a reason for hiding this comment

Gudahtt commented Aug 26, 2021

Gudahtt Aug 26, 2021

Choose a reason for hiding this comment

danjm Aug 26, 2021

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

pscott commented Sep 9, 2021 • edited Loading

darkwing commented Aug 16, 2021 •

edited by danjm

Loading

pscott commented Sep 9, 2021 •

edited

Loading