Skip to content
This repository was archived by the owner on Oct 19, 2024. It is now read-only.

Workaround for https://github.com/LedgerHQ/app-ethereum/issues/409 #2192

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

prestwich
Copy link
Collaborator

@prestwich prestwich commented Feb 25, 2023

makes chunk size dynamic to avoid running into this:
LedgerHQ/app-ethereum#409

Motivation

foundry-rs/foundry#4362

Solution

  • find the highest chunk size <= 255 that will not trigger the bug
  • drive-by: address some edge cases in payload handling that would have caused panics with incorrect ledger usage
  • drive-by: add tracing to ledger app (WIP)

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • [ n/a ] Breaking changes

@prestwich prestwich requested a review from gakonst February 25, 2023 01:40
@prestwich prestwich self-assigned this Feb 25, 2023
@prestwich prestwich marked this pull request as ready for review February 25, 2023 02:04
@prestwich prestwich changed the title WIP: Workaround for https://github.com/LedgerHQ/app-ethereum/issues/409 Workaround for https://github.com/LedgerHQ/app-ethereum/issues/409 Feb 25, 2023
@prestwich
Copy link
Collaborator Author

now ready for review 🖖

// workaround for https://github.com/LedgerHQ/app-ethereum/issues/409
// TODO: remove in future version
let chunk_size =
(0..=255).rev().find(|i| payload.len() % i != 3).expect("true for any length");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

finds the largest chunk size that does not leave 3 hanging bytes on the payload lol

Copy link
Owner

Choose a reason for hiding this comment

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

ridiculous

@prestwich prestwich force-pushed the prestwich/ledger-signer-workaround branch from 2ece0a5 to b1ec9ce Compare February 25, 2023 16:11
// workaround for https://github.com/LedgerHQ/app-ethereum/issues/409
// TODO: remove in future version
let chunk_size =
(0..=255).rev().find(|i| payload.len() % i != 3).expect("true for any length");
Copy link
Owner

Choose a reason for hiding this comment

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

ridiculous

Comment on lines +238 to +239
guard.record("index", index);
guard.record("chunk", hex::encode(chunk));
Copy link
Owner

Choose a reason for hiding this comment

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

TIL

@gakonst gakonst merged commit cc3156d into master Feb 27, 2023
@gakonst gakonst deleted the prestwich/ledger-signer-workaround branch February 27, 2023 20:13
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants