Feature/mintlayer app by OBorce · Pull Request #1 · mintlayer/mintlayer-ledger-app

OBorce · 2025-09-30T08:14:16Z

No description provided.

This reverts commit f72366c.

- All Pull requests are run in GitHub Actions - All pushes to `main` are run in GitHub Actions

Fix CI

`cargo-ledger` update

Update to latest SDK

use dedicated sections for each device

Moving to new manifest format

Code cleanup.

…s return type

messages/src/lib.rs

src/main.rs

src/handlers/sign_tx.rs

messages/src/lib.rs

ImplOfAnImpl · 2025-11-07T14:04:05Z

src/main.rs

+            (Ins::GET_VERSION, P1_GET_VERSION, P2_DONE) => Ok(Instruction::GetVersion),
+            (Ins::APP_NAME, P1_APP_NAME, P2_DONE) => Ok(Instruction::GetAppName),
+            (Ins::PUB_KEY, p1, P2_DONE) => {
+                let p1: PubKeyP1 = p1.try_into()?;
+                Ok(Instruction::GetPubkey {
+                    display: p1.display(),
+                })
+            }
+            (Ins::SIGN_TX, p1, P2_SIGN_MORE | P2_DONE) => Ok(Instruction::SignTx {
+                p1: p1.try_into()?,
+                more: value.p2 == P2_SIGN_MORE,
+            }),
+            (Ins::SIGN_MSG, P1_SIGN_START, P2_DONE)
+            | (Ins::SIGN_MSG, P1_SIGN_NEXT..=P1_SIGN_MAX_CHUNKS, P2_DONE | P2_SIGN_MORE) => {
+                Ok(Instruction::SignMessage {
+                    chunk: value.p1,
+                    more: value.p2 == P2_SIGN_MORE,
+                })
+            }
+            (
+                Ins::GET_VERSION | Ins::APP_NAME | Ins::PUB_KEY | Ins::SIGN_TX | Ins::SIGN_MSG,
+                _,
+                _,
+            ) => Err(StatusWord::WrongP1P2),
+            (_, _, _) => Err(StatusWord::InsNotSupported),


This is too messy. I think we should go with a more layered approach.

Since p2 values are universal, we can have a lower layer that handles APDUs and collects one or more of them into a single "low-level instruction", which would consist of ins, p1, and the data (possibly collected from multiple chunks).
This layer doesn't need to know what those ins/p1/data mean, it should just look at p2 - if p2 says "more chunks", then it should expect another APDU with the same ins/p1, collecting the received data in a "context" (this "context" should be separate from any other "context" that a higher-level layer may need).
Btw, on each APDU that says "more chunks" IMO it's better to return a special status word saying "expecting more chunks", so that the host can do sanity checks ensuring that it and the app are in sync.

Once a full "low-level instruction" (ins, p1 and full data) has been collected, it can be passed to the higher-level layer (also, perhaps p1 should just be named "parameter" at this point). This layer will interpret ins/p1, decode the data accordingly and produce a "high-level instruction" (need to come up with decent terminology, e.g. one of them may be called "command" and the other "instruction"), which should be a Rust enum containing all the data in the decoded form.

The "high-level instruction" is then passed to the handler, which returns Result<Response, Error>, where both Response and Error are Rust enums. This Result can then be converted to a status word and response data, which can be sent back to host.

src/handlers/sign_tx.rs

messages/src/lib.rs

ImplOfAnImpl

I didn't have enough time to look through all the code, I'll continue after returning from my vacation.

ImplOfAnImpl · 2025-12-19T16:22:19Z

.cargo/config.toml

+# By default, heap size is enforced to 8192 bytes.
+# Authorized values are [2048, 4096, 8192, 16384, 24576]
+# Uncomment the following lines to set the heap size to 4096 bytes for instance
+[env]
+HEAP_SIZE = "16384"


Plz remove the original comment and write our own, explaining why we need this particular heap size

Cargo.toml

ImplOfAnImpl · 2025-12-19T16:44:01Z

Cargo.toml

+bech32 = { version = "0.11", default-features = false, features = ["alloc"] }
+chrono = { version = "0.4", default-features = false, features = ["alloc"] }
+
+mintlayer-core-primitives = { git = "https://github.com/mintlayer/mintlayer-core-primitives", branch = "feature/ledger-coin-type" }


Plz don't forget to update this.
Also, IMO it's better to specify a specific revision and mention the commit message in a comment.

ImplOfAnImpl · 2025-12-19T16:45:27Z

.github/workflows/build_and_functional_tests.yml

@@ -0,0 +1,44 @@
+name: Build and run functional tests using ragger through reusable workflow


Rename the file to build_and_run_functional_tests.yml?

I didn't rename it as it comes from their boilerplate app conventions

ledger_app.toml

ImplOfAnImpl · 2025-12-19T18:06:01Z

Cargo.toml

+[profile.release]
+opt-level = 'z'
+lto = true


Another thing worth adding here is codegen-units = 1
E.g. I tried building for apex_p. Before:

# llvm-size /app/target/apex_p/release/mintlayer-app text data bss dec hex filename 145080 1024 19576 165680 28730 /app/target/apex_p/release/mintlayer-app

and with codegen-units = 1:

# llvm-size /app/target/apex_p/release/mintlayer-app text data bss dec hex filename 136224 1024 19576 156824 26498 /app/target/apex_p/release/mintlayer-app

I.e. the app will occupy 136224+1024 bytes of flash instead of 145080+1024.

P.S. the BSS section is the amount of RAM that the app's static data will occupy (it will be filled with zeroes at startup). I wonder whether this value is high and, if yes, whether we should look for ways to reduce it. IMO it'd make sense to at least look at other app's binaries (e.g. Ethereum and Bitcoin) and check their BSS sections (will need to build the apps from source for this).
One possible way of trying to reduce it would going through all dependencies and turning off all unneeded default features.

messages/src/lib.rs

src/main.rs

messages/src/lib.rs

ImplOfAnImpl

TBF I'm tired from your approach to PRs. From now on I want:

A clear indication of when a PR is ready for review. Normally, after changes have been requested, the PR should be changed to draft and when it comes back to "ready for review", then it's the indication that it is ready. If the PR never went to the draft state, then there should be some other indication, e.g. a comment "ready for review" or something. I will no longer look at PRs without clear indication that they're ready.
The code in a non-draft PR should be of production quality. No forgotten pieces of the original boilerplate app, no unused entities, outdated comments. Also, the usual general best practices should have already been applied - code duplication reduced to minimum, named constants instead of magic numbers etc.
(Btw, I'm not telling that the code should not have issues at all. But there is a huge difference between missing an issue and not bothering to even check).
To achieve #2 you have to review your own PRs. At least scroll through the diff looking for obvious issues/dirty code. E.g. I do this with my own PRs.
If you don't bother reviewing your own code, why should I. Doing code review is not pleasant and having to proof-read someones dirty code over and over again sucks all the fun from developer's life.
All review comments should be addressed, which means: applying the comment, initialing its discussion, either on GitHub or Slack, dropping it with a counter-comment, adding TODO to investigate it later, etc. Basically anything will do, except for silently ignoring it.
Again, accidents can happen, especialy in a large PR, but if they happen too often, then maybe the author of the PR doesn't care enough. And if he doesn't, then why should the reviewer care to write them, right?

ImplOfAnImpl · 2026-01-19T09:23:46Z

src/handlers/sign_tx.rs

+// Implement constructor for TxInfo with default values
+impl TxContext {


What?

P.S. check all comments and make sure they make sense

ImplOfAnImpl · 2026-01-19T09:56:16Z

src/handlers/sign_tx.rs

+        // version
+        tx_hasher
+            .update(&[version])
+            .map_err(|_| StatusWord::TxHashFail)?;


This is conceptually wrong - if we ever have Transaction::V2, it may have a completely different structure. So, instead of blindly encoding the version value, we should check if it's 1 and fail if it's not.
Also, the tx version should probably be an enum inside messages, e.g. TransactionVersion.

ImplOfAnImpl · 2026-01-19T10:41:40Z

messages/src/lib.rs

+    }
+
+    /// Creates a Vec of APDUs by chunking the data to MAX_ADPU_DATA_LEN
+    pub fn new_chunks(instruction_byte: u8, param1_byte: u8, data: &'a [u8]) -> Vec<Self> {


What was the reason for not returning ExactSizeIterator here?

ImplOfAnImpl · 2026-01-19T11:34:08Z

messages/src/lib.rs

+}
+
+#[derive(Encode, Decode)]
+pub struct GetVersionRespones {


typo - Respones

ImplOfAnImpl · 2026-01-19T11:34:14Z

messages/src/lib.rs

+}
+
+#[derive(Encode, Decode)]
+pub struct GetPublicKeyRespones {


typo - Respones
(same in the python tests)

ImplOfAnImpl · 2026-01-22T11:25:49Z

src/handlers/sign_tx.rs

+    DelegationStake,
+    DelegationWithdrawl,
+    CreateStakePool,
+    DecommissionStakePool,


Looks like this is never constructed.

Plz check other variants too.

ImplOfAnImpl · 2026-01-22T11:28:38Z

src/handlers/sign_tx.rs

+        TxOutput::CreateOrder(_) => {
+            ctx.tx_type = merge_tx_type(ctx.tx_type, TxType::CreateOrder);
+        }


Shouldn't increase_output_totals be called here as well.

If so, plz also make sure there is a unit test for this.

ImplOfAnImpl · 2026-01-22T11:35:01Z

src/main.rs

+}
+
+struct Context {
+    pub data: DataContext,


data_context

Though I'm not sure why the word "data" is here, probably the struct should be just Context and the field context.

ImplOfAnImpl · 2026-01-22T11:48:12Z

src/handlers/sign_message.rs

+    more: bool,
+    ctx: &mut DataContext,
+) -> Result<(), StatusWord> {
+    let ctx = match ctx {
+        DataContext::SignMessageContext(ctx) => ctx,
+        _ => return Err(StatusWord::WrongContext),
+    };
+    let chunk = comm.get_data().map_err(|_| StatusWord::WrongApduLength)?;
+
+    if ctx.message.len() + chunk.len() > MAX_MESSAGE_LEN {
+        return Err(StatusWord::TxWrongLength);
+    }
+
+    ctx.message.extend(chunk);
+
+    if more {
+        ctx.review_finished = false;
+        Ok(())


This is a mess.

more is always false here, so only 2 chunks can be received.

Why is chunking handled at this level? Why not handle it the same way as in the tx case, so that the entire data is passed into this function?

The data part of the first chunk is already in the data field of Command::SignMessage. But you ignore it and call comm.get_data() again.

ImplOfAnImpl · 2026-01-22T12:35:51Z

tests/application_client/mintlayer_command_sender.py

+        self.backend.exchange(
+            cla=CLA, ins=InsType.SIGN_MESSAGE, p1=P1.P1_START, p2=P2.P2_LAST, data=data
+        )
+        messages = split_message(message, MAX_APDU_LEN)


this is chunks not messages

Ericson2314 and others added 30 commits January 26, 2023 10:07

Revert "Temporarily point to our sdk"

e799ad4

This reverts commit f72366c.

Update to latest (merged) SDK with pending support

090156b

Fix CI

72022fa

- All Pull requests are run in GitHub Actions - All pushes to `main` are run in GitHub Actions

Merge branch 'pending-support' into fix-ci

fecb94f

Merge pull request #24 from obsidiansystems/fix-ci

91940d9

Fix CI

Add missing api_level flag. Cleanup config

dd9ec66

[ci] Use cargo-ledger

a6ba592

Merge pull request #26 from LedgerHQ/cargoledger_update

873a144

`cargo-ledger` update

Update README.md

c0ea47b

update to latest SDK

72162e7

fmt

edac388

Merge pull request #27 from LedgerHQ/update

7431fcf

Update to latest SDK

use dedicated sections for each device

f8b14e2

Merge pull request #28 from yogh333/feat/update_manifest

c1e2849

use dedicated sections for each device

[clean] Moving to new manifest format

43e50eb

Merge pull request #32 from LedgerHQ/update/manifest

f3d9f26

Moving to new manifest format

Add new main menu based on MultiPageMenu gadget.

3444069

Use MultiFieldReview gadget for tx review.

14aa875

Update authors, version and crate name in Cargo.toml

6da298a

Update .gitignore

9cfd8a2

Update app instruction handling and UI.

5d2ca17

Add Ragger tests and remove obsolete functional tests.

e423064

Add transaction signing APDU handling and display.

00d537f

Stack usage optim : use slices, not arrays in Tx struct.

0cab934

Code cleanup.

Add test snapshots for LNX / LNSP

11b9590

Remove unnecessary fields from config.toml

6e48413

Remove local version of nanos_ui in Cargo.toml

b134372

Format

b57d5bd

Use dependencies from the new SDK workspace.

a132cd4

Code cleanup.

Introduce new App Status Words enum and use it instead of io::Reply a…

337f1b9

…s return type

OBorce force-pushed the feature/mintlayer-app branch 8 times, most recently from ab1fa35 to 877082e Compare October 26, 2025 23:47

fix comments

2fa1c33

OBorce force-pushed the feature/mintlayer-app branch from 877082e to 2fa1c33 Compare October 28, 2025 09:02

fix missed comments

78bd2e2

ImplOfAnImpl requested changes Nov 7, 2025

View reviewed changes

OBorce added 4 commits November 12, 2025 10:06

Remove Get app name and version instructions

d65b520

fix streaming review starting too early

3d38d90

fix comments

d09c9c5

change messages

5b02b46

OBorce force-pushed the feature/mintlayer-app branch from 372ff0d to efb1812 Compare December 15, 2025 09:06

Move additional input info into along input messages

f0a534a

OBorce force-pushed the feature/mintlayer-app branch from efb1812 to f0a534a Compare December 15, 2025 09:34

OBorce added 3 commits December 16, 2025 18:41

fix tests

da0e723

flatten tx inputs for better memory efficiency

2a03f21

Use new TxInfoWithAdditionalInfo

5faeaaf

OBorce force-pushed the feature/mintlayer-app branch from 98949a4 to 5faeaaf Compare December 19, 2025 02:36

fix tests

b87b78a

ImplOfAnImpl reviewed Dec 19, 2025

View reviewed changes

OBorce added 2 commits December 24, 2025 12:36

fix comments

ad084ac

bump deps and toolchain

9ca7570

OBorce force-pushed the feature/mintlayer-app branch from bf565b1 to 9ca7570 Compare January 15, 2026 07:30

ImplOfAnImpl requested changes Jan 22, 2026

View reviewed changes

Move StatusWord to messages crate

dbc4134

		@@ -0,0 +1,44 @@
		name: Build and run functional tests using ragger through reusable workflow

		// Implement constructor for TxInfo with default values
		impl TxContext {

Conversation

OBorce commented Sep 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ImplOfAnImpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ImplOfAnImpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants