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

EIP-1559 - Provide support for Ledger #11786

Closed
wants to merge 2 commits into from

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Aug 5, 2021

Fixes: Provides Ledger support for EIP-1559

Depends on:

How to Test

  • Check out this PR locally
  • In the extension's node_modules/@metamask/eth-ledger-bridge-keyring/index.js file, change BRIDGE_URL to 'https://darkwing.github.io/eth-ledger-bridge-keyring', comment out the lines changed in this PR (Allow 1559 transactions with Ledger eth-ledger-bridge-keyring#93), and comment out the origin check in the _sendMessage method.

@darkwing darkwing requested a review from a team as a code owner August 5, 2021 21:49
@darkwing darkwing requested a review from ryanml August 5, 2021 21:49
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@darkwing
Copy link
Contributor Author

darkwing commented Aug 5, 2021

Just tested this briefly on the latest Ledger Live software update. I see Max Fee on the Ledger confirmation, but the value is always 0, so there's work to be done here. Transaction insta-fails.

@darkwing darkwing added the DO-NOT-MERGE Pull requests that should not be merged label Aug 6, 2021
@bayesianmind
Copy link

FWIW signing 1559 transactions works fine with ledger when used with mycrypto.com's ledger support. The ledger confirmation screen shows the max fee as nonzero, and the transaction goes through as expected.

I tried this patch and got the same results as you - Ledger shows 0 max gas, and the txn fails in metamask immediately.

ui/selectors/selectors.js Outdated Show resolved Hide resolved
@darkwing
Copy link
Contributor Author

darkwing commented Aug 17, 2021

I've updated the testing steps; to pin to the exact version of Ledger we need for Palm and EIP-1559 support, since updating to a newer version is a breaking change.

return !isHardwareWallet(state);
// Only Ledger supports 1559 at this times
const currentKeyring = getCurrentKeyring(state);
return currentKeyring && currentKeyring.type !== 'Trezor Hardware';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this literal 'Trezor Hardware' is used in a few places throughout the codebase (defined as KEYRING_TYPES.TREZOR in MetamaskController) this would be a good candidate for a shared constant I think

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@adonesky1
Copy link
Contributor

This PR was superceded by #11951

@adonesky1 adonesky1 closed this Sep 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO-NOT-MERGE Pull requests that should not be merged EIP-1559-v1.01
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants