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

Suggestion: declare explicit support of ASCII-only charset at SDK level #124

Open
dj8yfo opened this issue Jan 25, 2024 · 0 comments
Open

Comments

@dj8yfo
Copy link

dj8yfo commented Jan 25, 2024

this line https://github.com/LedgerHQ/ledger-device-rust-sdk/blob/master/ledger_device_sdk/src/ui/gadgets.rs#L831 can be changed to

enum FieldError {
    NonASCIIChar, 
}

pub fn show(&self) -> Result<bool, FieldError> {
...

Probably two additional checks can be added after line https://github.com/LedgerHQ/ledger-device-rust-sdk/blob/master/ledger_device_sdk/src/ui/gadgets.rs#L693 to assert that name and value fields of Field are comprised only of ASCII chars (https://doc.rust-lang.org/std/primitive.str.html#method.is_ascii), and returning Err(FieldError::NonASCIIChar) otherwise.

It appears to me that reflecting the fact, that non-ascii charsets are not supported, with types is more robust than displaying randomly blank chunks without any indication to user, why this happens.

MultiFieldReview::show() can be unwrapped as well at the call sites, if ledger-device-rust-sdk user doesn't want to bother with handling errors,
or the known error variants can be matched and handled, and e.g. a widget displayed to user, stating that Error: non-ASCII payload is not supported and a corresponding APDU status returned, refusing to continue to process.

@dj8yfo dj8yfo changed the title Suggestion: propagate error type in MultiFieldReview::show Suggestion: declare explicit support of ASCII-only charset at SDK level Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant