Skip to content

Replace precompiled ledger app binary with CI build and fork speculos-client with support for 2.4.0 starknet app#147

Open
MKowalski8 wants to merge 17 commits into
masterfrom
mkowalski/ledger-app-build
Open

Replace precompiled ledger app binary with CI build and fork speculos-client with support for 2.4.0 starknet app#147
MKowalski8 wants to merge 17 commits into
masterfrom
mkowalski/ledger-app-build

Conversation

@MKowalski8
Copy link
Copy Markdown
Member

@MKowalski8 MKowalski8 commented May 19, 2026

Towards foundry-rs/starknet-foundry#4221

Introduced changes

  • Remove precompiled binarires
  • Bump ledger app version to 2.4.0
  • Introudce new package in utils with support for 2.4.0 ledger app with speculos

Checklist

  • Linked relevant issue
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch 4 times, most recently from 5348be5 to 63c9a85 Compare May 20, 2026 09:14
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from 63c9a85 to 4756455 Compare May 20, 2026 09:31
Comment thread utils/speculos-client/Cargo.toml Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from fcf3569 to 4524ecc Compare May 20, 2026 10:00
@MKowalski8 MKowalski8 marked this pull request as ready for review May 21, 2026 11:45
@MKowalski8 MKowalski8 requested a review from a team as a code owner May 21, 2026 11:45
@MKowalski8 MKowalski8 changed the title Replace precompiled ledger app binary with CI build Replace precompiled ledger app binary with CI build and fork speculos-client with support for 2.4.0 starknet app May 21, 2026
git clone --depth 1 --branch nanox_2.7.0_2.4.0_sdk_v26.0.2 \
https://github.com/LedgerHQ/app-starknet /tmp/app-starknet
cd /tmp/app-starknet/starknet
cargo +${RUST_NIGHTLY} ledger build nanox
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Where is RUST_NIGHTLY set and why is it used here?

Copy link
Copy Markdown
Member Author

@MKowalski8 MKowalski8 May 25, 2026

Choose a reason for hiding this comment

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

It's set in the ledger-app-builder Dockerfile. And to run ledger build we need nightly version, because from what I've checked it uses -Z build-std which needs nightly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe add a comment that it's defined there?

Comment thread utils/speculos-client/src/lib.rs Outdated
Comment on lines +94 to +95
if line.contains("launcher: using default app name & version")
|| line.contains("[*] Env app version:")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems quite fragile, if output changes it will hang forever. Can we add some kind of a timeout here?

Copy link
Copy Markdown
Member Author

@MKowalski8 MKowalski8 May 25, 2026

Choose a reason for hiding this comment

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

Actually I've encountered this, but didn't want to make to much changes here. But yeah, it's a good idea, will do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I changed the logic a little bit, added some extra checks and better error handling as well

Comment thread utils/speculos-client/Cargo.toml Outdated
Comment thread utils/speculos-client/src/lib.rs Outdated
Comment thread utils/speculos-client/src/lib.rs Outdated
Comment thread .github/workflows/test.yaml Outdated
Comment thread utils/speculos-client/src/starknet_app.rs Outdated
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from bb6f02c to dc7c13c Compare May 27, 2026 14:00
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from ff2b8ba to 0468ddb Compare May 28, 2026 08:26
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch 2 times, most recently from 6f297a8 to a8f536c Compare May 28, 2026 09:50
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from a8f536c to 48b74b7 Compare May 28, 2026 10:12
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from a8ee712 to eab06b0 Compare May 28, 2026 10:31
@MKowalski8 MKowalski8 force-pushed the mkowalski/ledger-app-build branch from 7dd6202 to a164636 Compare May 28, 2026 13:10
git clone --depth 1 --branch nanox_2.7.0_2.4.0_sdk_v26.0.2 \
https://github.com/LedgerHQ/app-starknet /tmp/app-starknet
cd /tmp/app-starknet/starknet
cargo +${RUST_NIGHTLY} ledger build nanox
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe add a comment that it's defined there?

pub async fn set_automation(client: &SpeculosClient, rules: &[AutomationRule<'static>]) {
client.automation(rules).await.unwrap();
if rules.iter().any(|r| r == &ENABLE_BLIND_SIGN) {
client.click_button(Button::Right).await.unwrap();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be possible to return Result here instead of panic?

Copy link
Copy Markdown
Member Author

@MKowalski8 MKowalski8 May 29, 2026

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Let's add some doc comments in this file. Not to every single struct or method, but at least to the most important ones

@MKowalski8 MKowalski8 requested a review from ddoktorski June 1, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants