Skip to content

Re-export include_gif and ledger_secure_sdk_sys crates as modules #317

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Re-export include_gif and ledger_secure_sdk_sys crates as modules #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
48b4b3b
Re-export include_gif macro in the ledger_device_sdk module
bigspider Nov 12, 2025
55de6b8
Add a `sys` feature to re-export `ledger_secure_sdk_sys` as the `sys`…
bigspider Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ledger_device_sdk/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ nano_nbgl = [ "ledger_secure_sdk_sys/nano_nbgl" ]
debug_csdk = [ "ledger_secure_sdk_sys/debug_csdk" ]
io_new = [] # switch to new 'io' module

# When enabled, this feature re-exports the `ledger_secure_sdk_sys` crate containing the
# Ledger C SDK bindings through the `sys` module. Use it to access low-level (unsafe) FFI
# functions, types, and constants directly from the underlying C SDK.
# It is preferred to not enable this feature if possible, using instead the safe
# abstractions in `ledger_device_sdk`, but it is provided for advanced use cases.
sys = []

default = [ "heap" ]

[lints.rust.unexpected_cfgs]
Expand Down
11 changes: 10 additions & 1 deletion ledger_device_sdk/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,19 @@ pub fn exiting_panic(_info: &PanicInfo) -> ! {
ledger_secure_sdk_sys::exit_app(0);
}

// re-export exit_app
// Re-export exit_app
pub use ledger_secure_sdk_sys::buttons;
pub use ledger_secure_sdk_sys::exit_app;

// Re-export include_gif macro
pub use include_gif::include_gif;

Copy link
Contributor

@yogh333 yogh333 Nov 12, 2025

Choose a reason for hiding this comment

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

Yes it is a pretty good idea but it means that include_gif is no more an independent package published on crates.io right?
And for all existing apps, after updating to this version of the Rust SDK which includes the include_gif as a module the include_gif dependency shall be removed?
Also all cargo update include_gif (in GitHub workflow, deployment workflow) shall be removed then.

Copy link
Contributor Author

@bigspider bigspider Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is still exported as an independent package; this incrementally makes it available to apps.

And for all existing apps, after updating to this version of the Rust SDK which includes the include_gif as a module the include_gif dependency shall be removed?

Yes, I think it should be removed from the boilerplate (and can be removed from existing apps, after making sure the import is fixed accordingly).
It always makes more sense for apps to use the same version the sdk is using, or you end up with two versions if there's a mismatch.

Good point about the workflows, but I guess it should be only removed once the dependency is removed from existing apps?

Copy link
Contributor

@yogh333 yogh333 Nov 12, 2025

Choose a reason for hiding this comment

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

After checking, include_gif should not be used in the Rust SDK: all internal glyphs are retrieved from the C SDK (any update of those glyphs in the C SDK will be seamlessly inherited by the Rust apps). The only reason include_gifis used by the Rust SDK is for 2 BAGL glyphs used by few Rust apps:

Those icons are used on Nano X/S+ by Radix, MobileCoin, Alephium, Sui while they should use the C SDK corresponding glyphs (VALIDATE_14_ICON and CROSS_MARK_ICON) but it triggers UI modifications.

include_gif should be only used by apps and thus there is no need/reason to make this package as a Rust SDK module. Once all the apps running on BAGL will be migrated on NBGL, all BAGL glyphs will be removed and the include_gifdependency for the Rust SDK will be removed as well.

Copy link
Contributor Author

@bigspider bigspider Nov 12, 2025

Choose a reason for hiding this comment

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

Don't all apps need to use it at least once for the icon show in NbglHomeAndSettings?

Also, the possibility of showing other icons in custom UX flows seems potentially useful.

Copy link
Contributor

@yogh333 yogh333 Nov 12, 2025

Choose a reason for hiding this comment

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

Yes all apps need to include include_gif as a dependency, but to me it is a separate package from the Rust SDK. Same conclusion if custom UI/UX is needed at application level. Actually, I do not see why we should make this package linked with the Rust SDK, it is just an additional package that Ledger provides to build Rust applications. The fact that nowadays the Rust SDK is using include_gif is to me temporary.
The only reason I see to use include_gif in the Rust SDK could be to define new UX flows in the Rust SDK, different from the ones already proposed by the C SDK (and Rust SDK leverages). But, from the embedded apps point of view, this is not what we want, as the UI/UX in between C and Rust apps shall be strictly the same (that's unfortunate but it is a very hard requirement 😢 )

Copy link
Contributor Author

@bigspider bigspider Nov 12, 2025

Choose a reason for hiding this comment

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

include_gif is already not really standalone, as it's tightly coupled with how Ledger devices (BAGL/NBGL) represents images in-memory. In fact it was changed just 2 months ago for NBGL support - which wouldn't happen if it was an independent package.

The rationale for including it as a feature of ledger_device_sdk is that it would be nice for developers if by just including ledger_device_sdk they get all they need to build Ledger apps. Even if the Rust SDK later stops using include_gif, it's still less hassle for developers and simplifies app dependencies.

It also has impact on the build pipeline, as exemplified by the various crate patches in the CI (which is ugly and footgun-prone, and we should try to remove ASAP).

Copy link
Contributor

@yogh333 yogh333 Nov 12, 2025

Choose a reason for hiding this comment

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

Ok I think I see what you mean, and I agree. I need to make further tests to evaluate the impacts of this modification.
Thank you!

// Re-export the underlying Ledger C SDK bindings when the `sys` feature is enabled.
// This provides direct (unsafe) FFI access. Prefer the safe abstractions in this crate
// when possible.
#[cfg(feature = "sys")]
pub use ledger_secure_sdk_sys as sys;

use ledger_secure_sdk_sys::{pic_rs, pic_rs_mut};

/// Helper macro that sets an external panic handler
Expand Down