More documents, less magic numbers, more tests

Author: polarker

app/src/blake2b_hasher.rs

@@ -4,6 +4,7 @@ use ledger_secure_sdk_sys::*;
 pub const BLAKE2B_HASH_SIZE: usize = 32;
 pub struct Blake2bHasher(cx_blake2b_s);
 
+// A wrapper around the Ledger SDK's blake2b implementation
 impl Blake2bHasher {
     pub fn new() -> Self {
         let mut v = cx_blake2b_t::default();

app/src/handler.rs

@@ -57,6 +57,7 @@ pub fn handle_apdu(
         return Err(ErrorCode::BadCla.into());
     }
 
+    // Common instructions
     match ins {
         Ins::GetVersion => {
             let version_major = env!("CARGO_PKG_VERSION_MAJOR").parse::<u8>().unwrap();
@@ -79,8 +80,8 @@ pub fn handle_apdu(
 
             println("raw path");
             println_slice::<PATH_HEX_LENGTH>(raw_path);
-            let p1 = apdu_header.p1;
-            let p2 = apdu_header.p2;
+            let p1 = apdu_header.p1; // Group number: 0 for all groups
+            let p2 = apdu_header.p2; // Target group
             let (pk, hd_index) = derive_pub_key(&mut path, p1, p2)?;
 
             let need_to_display = data[PATH_LENGTH] != 0;
@@ -115,6 +116,8 @@ pub fn handle_apdu(
                     return Ok(());
                 }
                 Ok(()) => {
+                    // The transaction is signed when all the data is processed
+                    // The signature is returned in the response
                     let sign_result = tx_reviewer
                         .approve_tx()
                         .and_then(|_| sign_tx_context.sign_tx());
@@ -136,6 +139,10 @@ pub fn handle_apdu(
     Ok(())
 }
 
+// The transaction is split into multiple APDU commands
+// The first APDU command contains the path and token metadata
+// The subsequent APDU commands contain the transaction data
+// The transaction data is processed in chunks
 fn handle_sign_tx(
     apdu_header: &ApduHeader,
     data: &[u8],
@@ -145,7 +152,10 @@ fn handle_sign_tx(
     match apdu_header.p1 {
         0 if data.len() < FIRST_FRAME_PREFIX_LENGTH => Err(ErrorCode::BadLen),
         0 => {
+            // handle the path
             sign_tx_context.init(&data[..PATH_LENGTH])?;
+
+            // handle the token metadata
             let token_size = data[FIRST_FRAME_PREFIX_LENGTH - 1];
             if token_size > MAX_TOKEN_SIZE {
                 return Err(ErrorCode::InvalidTokenSize);
@@ -170,6 +180,8 @@ fn handle_sign_tx(
     }
 }
 
+// Check the token metadata version
+// The token metadata version should be 0 for now
 fn check_token_metadata(token_size: u8, token_metadata: &[u8]) -> Result<(), ErrorCode> {
     for i in 0..token_size {
         let version_index = (i as usize) * TOKEN_METADATA_SIZE;

app/src/ledger_sdk_stub/nvm.rs

@@ -4,6 +4,9 @@ use crate::error_code::ErrorCode;
 
 pub const NVM_DATA_SIZE: usize = 2048;
 
+// The following code is from ledger-rust-sdk
+// We've made modifications here to add the `get_mut` functions to `NVMData`
+
 #[allow(clippy::upper_case_acronyms)]
 #[repr(align(64))]
 pub struct NVM<const N: usize>(pub [u8; N]);

app/src/main.rs

@@ -24,10 +24,12 @@ mod ui;
 
 ledger_device_sdk::set_panic!(ledger_device_sdk::exiting_panic);
 
+// This function is the app entry point
 #[no_mangle]
 extern "C" fn sample_main() {
     let mut comm = io::Comm::new();
 
+    // Initialize the sign tx context and tx reviewer
     let mut sign_tx_context: SignTxContext = SignTxContext::new();
     let mut tx_reviewer: TxReviewer = TxReviewer::new();
 

app/src/public_key.rs

@@ -5,26 +5,17 @@ use ledger_device_sdk::ecc::SeedDerive;
 use ledger_device_sdk::ecc::{ECPublicKey, Secp256k1};
 use ledger_device_sdk::io::Reply;
 use utils::base58::base58_encode_inputs;
-use utils::{djb_hash, xor_bytes};
+use utils::{check_group, djb_hash, xor_bytes};
 
-pub const TOTAL_NUMBER_OF_GROUPS: u8 = 4;
-
-fn check_group(group_num: u8, target_group: u8) -> Result<(), Reply> {
-    if group_num == 0 && target_group == 0 {
-        return Ok(());
-    }
-    if target_group >= group_num || group_num != TOTAL_NUMBER_OF_GROUPS {
-        return Err(ErrorCode::BadP1P2.into());
-    }
-    Ok(())
-}
+const RAW_PUBKEY_SIZE: usize = 65;
+const COMPRESSED_PUBKEY_SIZE: usize = 33;
 
 pub fn derive_pub_key(
     path: &mut [u32],
     group_num: u8,
     target_group: u8,
 ) -> Result<(ECPublicKey<65, 'W'>, u32), Reply> {
-    check_group(group_num, target_group)?;
+    check_group::<Reply>(group_num, target_group, ErrorCode::BadP1P2.into())?;
     if group_num == 0 {
         let pub_key = derive_pub_key_by_path(path)?;
         Ok((pub_key, path[path.len() - 1]))
@@ -40,6 +31,8 @@ pub fn derive_pub_key_by_path(path: &[u32]) -> Result<ECPublicKey<65, 'W'>, Repl
     Ok(pk)
 }
 
+// Derive a public key for a specific group from a path
+// The path is incremented until the target group is found
 fn derive_pub_key_for_group(
     path: &mut [u32],
     group_num: u8,
@@ -55,9 +48,9 @@ fn derive_pub_key_for_group(
 }
 
 pub fn hash_of_public_key(pub_key: &[u8]) -> [u8; BLAKE2B_HASH_SIZE] {
-    assert!(pub_key.len() == 65);
-    let mut compressed = [0_u8; 33];
-    compressed[1..33].copy_from_slice(&pub_key[1..33]);
+    assert!(pub_key.len() == RAW_PUBKEY_SIZE);
+    let mut compressed = [0_u8; COMPRESSED_PUBKEY_SIZE];
+    compressed[1..COMPRESSED_PUBKEY_SIZE].copy_from_slice(&pub_key[1..COMPRESSED_PUBKEY_SIZE]);
     if pub_key.last().unwrap() % 2 == 0 {
         compressed[0] = 0x02
     } else {

app/src/sign_tx_context.rs

@@ -1,5 +1,7 @@
 use ledger_device_sdk::io::ApduHeader;
-use utils::{buffer::Buffer, decode::StreamingDecoder, deserialize_path, types::UnsignedTx};
+use utils::{
+    buffer::Buffer, decode::StreamingDecoder, deserialize_path, types::UnsignedTx, PATH_LENGTH,
+};
 
 use crate::ledger_sdk_stub::nvm::{NVMData, NVM, NVM_DATA_SIZE};
 use crate::ledger_sdk_stub::swapping_buffer::{SwappingBuffer, RAM_SIZE};
@@ -12,6 +14,7 @@ use crate::{
     error_code::ErrorCode,
 };
 
+// The NVM data is used for SwappingBuffer to store temporary data in case RAM is not enough
 #[link_section = ".nvm_data"]
 static mut DATA: NVMData<NVM<NVM_DATA_SIZE>> = NVMData::new(NVM::zeroed());
 
@@ -22,6 +25,9 @@ enum DecodeStep {
     Complete,
 }
 
+// The context for signing a transaction
+// It keeps track of the current step, the transaction decoder, the path, and the device address
+// A streaming decoder is used to decode the transaction in chunks so that it can handle large transactions
 pub struct SignTxContext {
     pub path: [u32; 5],
     tx_decoder: StreamingDecoder<UnsignedTx>,
@@ -34,7 +40,7 @@ pub struct SignTxContext {
 impl SignTxContext {
     pub fn new() -> Self {
         SignTxContext {
-            path: [0; 5],
+            path: [0; PATH_LENGTH],
             tx_decoder: StreamingDecoder::default(),
             current_step: DecodeStep::Init,
             hasher: Blake2bHasher::new(),
@@ -43,6 +49,7 @@ impl SignTxContext {
         }
     }
 
+    // Initialize the context with the path
     pub fn init(&mut self, data: &[u8]) -> Result<(), ErrorCode> {
         deserialize_path(data, &mut self.path, ErrorCode::HDPathDecodingFailed)?;
 
@@ -58,11 +65,13 @@ impl SignTxContext {
         self.current_step == DecodeStep::Complete
     }
 
+    // Get the transaction ID by finalizing the hash
     pub fn get_tx_id(&mut self) -> Result<[u8; BLAKE2B_HASH_SIZE], ErrorCode> {
         assert!(self.is_complete());
         self.hasher.finalize()
     }
 
+    // Sign the transaction by signing the transaction ID
     pub fn sign_tx(&mut self) -> Result<([u8; 72], u32, u32), ErrorCode> {
         let tx_id = self.get_tx_id()?;
         sign_hash(&self.path, &tx_id)
@@ -75,6 +84,7 @@ impl SignTxContext {
     ) -> Result<(), ErrorCode> {
         while !buffer.is_empty() {
             match self.tx_decoder.step(buffer) {
+                // New transaction details are available
                 Ok(true) => {
                     tx_reviewer.review_tx_details(
                         &self.tx_decoder.inner,
@@ -90,42 +100,51 @@ impl SignTxContext {
                         self.tx_decoder.reset_stage();
                     }
                 }
+                // No new transaction details are available
                 Ok(false) => return Ok(()),
                 Err(_) => return Err(ErrorCode::TxDecodingFailed),
             }
         }
         Ok(())
     }
 
-    fn decode_tx(&mut self, data: &[u8], tx_reviewer: &mut TxReviewer) -> Result<(), ErrorCode> {
-        if data.len() > (u8::MAX as usize) {
+    // Decode a transaction chunk
+    fn decode_tx(
+        &mut self,
+        tx_chunk: &[u8],
+        tx_reviewer: &mut TxReviewer,
+    ) -> Result<(), ErrorCode> {
+        if tx_chunk.len() > (u8::MAX as usize) {
             return Err(ErrorCode::BadLen);
         }
-        let mut buffer = Buffer::new(data, &mut self.temp_data);
+        let mut buffer = Buffer::new(tx_chunk, &mut self.temp_data);
         let result = self._decode_tx(&mut buffer, tx_reviewer);
-        self.hasher.update(data)?;
+        self.hasher.update(tx_chunk)?;
         result
     }
 
+    // Handle a transaction data chunk
     pub fn handle_data(
         &mut self,
         apdu_header: &ApduHeader,
-        tx_data: &[u8],
+        tx_data_chunk: &[u8],
         tx_reviewer: &mut TxReviewer,
     ) -> Result<(), ErrorCode> {
         match self.current_step {
             DecodeStep::Complete => Err(ErrorCode::InternalError),
             DecodeStep::Init => {
+                // The first chunk of the transaction
                 if apdu_header.p1 == 0 {
                     self.current_step = DecodeStep::DecodingTx;
-                    self.decode_tx(tx_data, tx_reviewer)
+                    self.decode_tx(tx_data_chunk, tx_reviewer)
                 } else {
                     Err(ErrorCode::BadP1P2)
                 }
             }
             DecodeStep::DecodingTx => {
+                // The subsequent chunks of the transaction
                 if apdu_header.p1 == 1 {
-                    self.decode_tx(tx_data, tx_reviewer)
+                    self.decode_tx(tx_data_chunk, tx_reviewer)
                 } else {
                     Err(ErrorCode::BadP1P2)
                 }