feat: filter out change note on certain scenarios, add from address t…

…o review flow (#91)

* Add "From" rendering

* Filter change outputs

* Fix rust tests and compilation errors

* Update rust test snapshots

* Add logging

* Add change address rust test

* Update snapshots

* Update sdk to latest

* fix debug feature flag

* fix clippy

* Update snapshots

* Remove unused testing data(duplicated)

* Only filter out change_Address in normal mode and enhance our filtering to be more robust without depending on field ordering

* Update method to build transaction to use alternative receiver

* Update tests and snapshots

* Update snapshots

* In normal mode allow change output rendering if it is the only one

* Fix buildTx to remove previous ambiguity and update tests

* Update snapshots

* Add test to check change output rendering if it is the only one

app/Cargo.toml

@@ -10,7 +10,7 @@ path = "bin-app/app.rs"
 
 [dependencies]
 # ledger_device_sdk = { git = "https://github.com/Zondax/ledger-device-rust-sdk", rev = "446bd4c11c328e422e68f056b08ed38952a0853f", optional = true }
-ledger_device_sdk = { version = "1.18.3", optional = true }
+ledger_device_sdk = { version = "1.18.4", optional = true }
 #ledger_device_sdk = { path="../ledger-device-rust-sdk/ledger_device_sdk" , optional = true}
 include_gif = "1.2.0"
 serde = { version = "1.0.192", default_features = false, features = ["derive"] }
@@ -53,6 +53,7 @@ lazy_static = { version = "1.5.0" }
 insta = { version = "1", features = ["glob"] }
 serde_json = "1.0.85"
 rand = "0.8.5"
+once_cell = "1.20.2"
 
 [profile.release]
 lto = true

app/src/parser/note.rs

@@ -149,13 +149,14 @@ impl Note {
     pub fn review_fields(
         &self,
         token_list: &TokenList,
-        fields: &mut Vec<(String, String)>,
-    ) -> Result<(), ParserError> {
+    ) -> Result<Vec<(String, String)>, ParserError> {
         use lexical_core::FormattedSize;
 
+        let mut fields = Vec::new();
+
         zlog_stack("Note::review_fields\n");
         // Format To:
-        let to = String::from("To ");
+        let to = String::from("To");
         let address = hex::encode(self.owner.public_address());
         fields.push((to, address));
 
@@ -199,6 +200,6 @@ impl Note {
             fields.push((label, asset_id));
         }
 
-        Ok(())
+        Ok(fields)
     }
 }

app/src/parser/snapshots/ironfish_dkg__parser__transaction__review_transaction_test__tx_ui.snap

@@ -5,12 +5,13 @@ input_file: src/parser/testvectors/transaction_simple.json
 ---
 [
     "Tx Version": "V1",
-    "To ": "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd",
+    "From": "b26388e8e7c12c80c7f20a8310137d4eb6b4bf3674e8a702b26ff4955f3d58c0",
+    "To": "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd",
     "Amount": "0.0000004 IRON",
-    "To ": "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd",
+    "To": "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd",
     "Raw Amount ": "5",
     "AssetId ": "da40fc530a12327a1c1c367b8bd66ada35100501228cdb355b8fb6c40d048a64",
-    "To ": "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd",
+    "To": "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd",
     "Amount": "0.00000001 IRON",
     "Fee": "0.00000001 IRON",
     "Expiration": "0",

app/src/parser/transaction.rs

@@ -21,6 +21,17 @@ use crate::{
     utils::int_format::intstr_to_fpstr_inplace,
 };
 
+#[cfg(test)]
+use once_cell::sync::Lazy;
+
+#[cfg(test)]
+use std::sync::RwLock;
+
+#[cfg(test)]
+static FROM_ADDR: Lazy<RwLock<String>> = Lazy::new(|| {
+    RwLock::new("b26388e8e7c12c80c7f20a8310137d4eb6b4bf3674e8a702b26ff4955f3d58c0".to_string())
+});
+
 mod burns;
 mod mints;
 mod outputs;
@@ -136,11 +147,44 @@ impl<'a> Transaction<'a> {
         self.outputs.iter()
     }
 
+    fn get_our_address() -> Result<String, IronfishError> {
+        #[cfg(all(feature = "ledger", not(test)))]
+        use crate::crypto::{derive_multisig_account, multisig_to_key_type};
+
+        #[cfg(all(feature = "ledger", not(test)))]
+        {
+            let from = derive_multisig_account(None).map_err(|_| IronfishError::InvalidSecret)?;
+            let from = multisig_to_key_type(&from, 0u8).map_err(|_| IronfishError::InvalidData)?;
+            Ok(hex::encode(from))
+        }
+
+        #[cfg(test)]
+        {
+            Ok(FROM_ADDR.read().unwrap().to_owned())
+        }
+
+        #[cfg(not(any(test, feature = "ledger")))]
+        {
+            Ok("b26388e8e7c12c80c7f20a8310137d4eb6b4bf3674e8a702b26ff4955f3d58c0".to_string())
+            // Or handle this case differently
+        }
+    }
+
+    #[cfg(test)]
+    fn set_from_address(from: &str) {
+        if let Ok(mut addr) = FROM_ADDR.write() {
+            *addr = from.to_string();
+        }
+    }
+
     #[inline(never)]
     pub fn review_fields(
         &self,
         ovk: &OutgoingViewKey,
     ) -> Result<Vec<(String, String)>, IronfishError> {
+        #[cfg(ledger)]
+        use crate::crypto::{derive_multisig_account, multisig_to_key_type};
+
         zlog_stack("Transaction::review_fields\n");
 
         let mut fields = Vec::new();
@@ -151,9 +195,27 @@ impl<'a> Transaction<'a> {
             self.tx_version.as_str().to_string(),
         ));
 
-        let token_list = get_token_list()?;
+        // Add from
+        let from = Self::get_our_address()?;
 
-        for output in self.outputs.iter() {
+        fields.push((String::from("From"), from.clone()));
+
+        let token_list = get_token_list()?;
+        #[cfg(feature = "ledger")]
+        let expert_mode = crate::nvm::settings::Settings.app_expert_mode();
+        #[cfg(not(feature = "ledger"))]
+        // useful for rust unit testing
+        // here we do not filter out
+        // new tokens, it is done in note
+        // so here we just set expert_mode
+        // to false to control the rendering
+        // of having only one outputs, to test that
+        // in rust
+        let expert_mode = false;
+
+        let is_one_output = self.num_outputs() == 1;
+
+        'note: for output in self.outputs.iter() {
             // Safe to unwrap because MerkleNote was also parsed in outputs from_bytes impl
             let Ok(merkle_note) = output.note() else {
                 return Err(IronfishError::InvalidData);
@@ -163,14 +225,31 @@ impl<'a> Transaction<'a> {
             let note = merkle_note.decrypt_note_for_spender(ovk)?;
 
             // Now process amount and fees
-            note.review_fields(&token_list, &mut fields)?;
+            let note_fields = note.review_fields(&token_list)?;
+
+            // Is this note a change output that goings back to us?
+            let is_change_note = note_fields
+                .iter()
+                .any(|(key, value)| key.contains("To") && value == &from);
+
+            // Only render items that does not belong to us
+            // except if expert mode is enable, in this case show everything
+            // and if this is the only output even if it is a change
+            // we display that
+            if is_change_note && !expert_mode && !is_one_output {
+                continue 'note;
+            }
+
+            fields.extend(note_fields);
         }
+
         // Safe to unwrap, IRON is the oficial token
         let Some(token) = token_list.toke_by_symbol("IRON") else {
             return Err(IronfishError::InvalidData);
         };
 
         let mut buffer = [0; lexical_core::BUFFER_SIZE];
+
         // Add fee
         lexical_core::write(self.fee, &mut buffer[..]);
         let raw = intstr_to_fpstr_inplace(&mut buffer[..], token.decimals as usize)?;
@@ -277,6 +356,7 @@ mod review_transaction_test {
 
     // testing data for unit tests
     const TRANSACTION: &str = "0101000000000000000300000000000000010000000000000000000000000000000100000000000000000000003475b26a991a739f6b77dd7bce822efa46d355a28cf0e4dc9c93b6ababf073c5ad26e3f59270401ff48e7ba11d800eaea4d91dc89d4ccf0975afec009dcebb07ad26e3f59270401ff48e7ba11d800eaea4d91dc89d4ccf0975afec009dcebb0795aef72203054fbfa24ffd1c375e6d69827111b74805477b38ef4932d143c6a44efc37619fbedc3a1b46d5613202ac6baf75caadb2ac6b6c9db0b02acf8698a1f51eb6a1a1dbebf7fa0b034803716d103c96e9893180cc971824bc2b978e1b1600ebd35de5d6b921b2ce8aa4c03ef7312c9d6efc7e259f4dd68d5c7b632a88bc259386faae6cea9e44e232a6cbf1079486a29e9d622e3c25e1985155226c4d48342be389600dc829f89aac5a81c444fd880f5b5ba1541b9434d620543a6e8be7eef40fe52914631d18a7d4dfb1c41beed6c2b7a51efd985f2e210059f36f6000ec8ee990b1228496c0d1140767f2aae76d79e09f5f777ff5af0f89ef6aefe7337805000045e9b744ed2afca6615aa6ba00dcf578979391b219ca05e90447abf21315a9be00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000b9032b6225f92f518e019f07373891b6524df7bd3e25afc0b0fb7f5ca2da049c419860e0eb24aff16d830bc21061662393a76e80b2ef434d6f9bbfc3926644e933210d159074e80b368c16312710226160c38e0b05e69a8f353c5de3e0e6fd1e03892c58d7e8eeb5604aab425b445d86e9357341e2a175b8835b99653d75ba42c11500beeb61c90523a7dee736d73ab2a6e496b7aa8241ae8bd222475f3e0b510b0f5a57c5a97583d342d4c8740f66fb856d730caafa608296b2b02aa7bd4a99499b3cde309b75e0200df587d09952547f29046abc4fdd47714860d09f9cdcf3f3c24fd8d8fd0e0d2f83f1191ff59f37fa811d1a4c90faf70d4da89ef228ce6d8d0af19c715cae4d74fe07d3d66c473dff6799bc3d8979fafd5a9cef9b1d7650a1f488cd3478146e27075c1b1c73697ba64e94b37d13f29d22068ef58a7a721138213f218a11d1ab4b88d41420b40a010c9cbf2e10f00f7ae52dd54eeb9b716fac76db7d364b5cf43d278b61a0863234c03485364347505af9c86b8d582c453c1c98e52882d08e49d496791a899796ea3e5773435edc97caadffcc579341474cc31b87e6b19b0d526618585a6693184f903512ef641eda23f30e3a45782e95d1c3529230566b78aa1421834c58d0d44ab79467aff475858a38cbd67d4c4af4ed0cf677a063fea0b38b193ab6a18d81b3848f941d61b46bd86f348c613a9b0dc18610fae24d7328209168bb05d58d1085496a5a4769da339fa3ef1463956da25ccaff94a58beff6417d3f94d07eece27b8ca1db74ce29cce9a40556f8967d5ebb58da4efd01e4fb9b27578fc8e883320cff5f36a1b69706816cabdaff3ba9cb06756d8c37d760419d05bd9b8ffa84ed6ea98b952f0ea2e927ba0782e90a949b8821c305c20db46d66c00b48fc4150eeff0bc07a6fd890c80a802657573703a1b7aa9bd3b6eda9ad2adce2f162d1117aaf08093c779dd4db20c94a48e62357ade7daf5284d3b4d2f6885dc03a34b48aa2890ab4c60128769bf8bef2ee7a06f4d552eaed15c4df1da5ccf3762f37b818c9d62f7464416bc40f59c4f3c6299a929a67dd3d09e00d1bd68c16d38ee66e34b54272e0fdd2eb74c91e2eb66d4db7d061ddedb0b88f709fa9f027b00a5fbacb4f70f48612b11caf119866afea8a05de5b9e63b096e45634d7d68aef803cec2d21f91a55bea60be8abb233afe7dbbc415546945350432dc858064d42b82b0f0397fca660dddd45e9d95898d98d2fcbf2eb0c5de920aed36f3f56b511dd1ca5551a14b5d1b235be781c7c1a6f3297d4bccf83abb11bcaca33e12ea204c06849b39dae12e0cee817e0cbde1b1d1906b88cac1d96ed59c1377a0f7eac1a4efe9d2cafbcfd0c270f0d8311ac9839ad60cc834c1785e2796485dd844fa0694091206f8d3563830c2160a5510a41970da6cd5a0f810d36df27339db81152a7c2acc2f958f8d847ef2a9b18862eb2ebe6e2a635f905db4416d594d25268c61d163ee17d0b9052a6103d70139ec53b9f8328d5e3eed90541e494ad15da810b71fba9543198a4505e464608367a7fc83ae6ac1f7a63f3594d20452d928b86bbdccda5f9257f615eef5f05a7513cff8d074860980bca8873d387bb23894694650c9adad3ac1ba0ef617dbcec9946d874516131944e6e3a10eb017d696a20eaf9af5ffbc83b8f2db1e2cf82bfa1a2072523b844108d8fa9c980443c92121b2a2d47114b4dec3b0dbdab9ebd96295443d8a587682a044c2cc7ba72e9842ffb1156b7c774efdf0cdb5f96be7ad6cb1c361e3d7ee645c0c57dd23b4d247d940063cfa8a4afb72b671cc1d6f9037aac0cab8852d606d5616f41a3f8d7901176060a75e4c9e18d65cd2679fe5083c6e40e247c74f29b4ab9ea38827c7f4d1f1fac5a7409262374adcf69975be903a8561b83e37980567fd179e0f7eab72cb77f5fab2e44e49f8cb75f126b5d60cdbcacd00ab422d48d32d2db75442549ed7fe4b220ca969a9d8a4248cff3725f34cb64c6a59e73d8e841e5995075f32feec14d2954af1afec302d30d24214025e833f68676b1242f27dba75dd53426b46ea4fff0163ba0cce0e6e9ceecd5191199f5c9b867e62a4ab18781f7b440e6d9470ba05f09f0ed1062a745f4cb952ca862c91c3d3e88779f51c43bdc0973f03b5266f3296772fe19ff078ad2b7f237fcbbe47e4d2ad26e3f59270401ff48e7ba11d800eaea4d91dc89d4ccf0975afec009dcebb07b1ed0a33d8f1fb025d2fdaa5af2801bdf2aff84d3b69bdbf6882b424803170ff6db19d9e05cff32d44dbb9c5d6c5e00683956df74a3db738668124f64928c5e10fe1b733def7f86b6532c9150286d71038490e719cd4dd271e883bcd278750140ba5b3dbd716a40d94a9eeed141ae4a6d007b8cfd43a175a6c862df720bd03f7c38706371ab73ab00489294f72c6b18eb470d8f395fac71672ae4b2dd9afc8cbb986aaab0a15954d29b323fa21c13772068078845159b7b52a5bf766cea25e2640fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd54657374636f696e00000000000000000000000000000000000000000000000041207265616c6c7920636f6f6c20636f696e00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001050000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000389e2f71e4e9bc3e37a8b3a5b49432f7a93574eced25fce9add210b41203ed42e414a3468188977dfa4f1bbb2e36dc1bce38e759b8a54c85bea6d75a2aeea008";
+    const TO_ADDR: &str = "40fae059d8ee3361b7a08429867254523f937c7654ae6fe7b188c1f0da57e9cd";
     const TX_HASH: &str = "722c8f5e8e02097b821c9c03be3165c3cecf2262f31cf2e31a10bada2fe1b033";
     const OVK: &str = "49bad8395ef448eb0048af132b5c942579024736d4c3cfd685b241b994f8f8e5";
 
@@ -314,6 +394,14 @@ mod review_transaction_test {
     }
 
     #[test]
+    fn review_transaction() {
+        // review fields
+        tx_ui();
+
+        // check change address work
+        test_change_address();
+    }
+
     #[cfg_attr(miri, ignore)]
     fn tx_ui() {
         insta::glob!("testvectors/*.json", |path| {
@@ -350,4 +438,31 @@ mod review_transaction_test {
             unsafe { with_leaked(data, test) };
         });
     }
+
+    fn test_change_address() {
+        let tx = hex::decode(TRANSACTION).unwrap();
+        let (_, tx) = Transaction::from_bytes(&tx).unwrap();
+        assert_eq!(tx.num_spends(), 1);
+        assert_eq!(tx.num_outputs(), 3);
+        assert_eq!(tx.num_mints(), 1);
+        assert_eq!(tx.num_burns(), 0);
+
+        let ovk = hex::decode(OVK).unwrap();
+        let ovk = OutgoingViewKey::new(ovk.try_into().unwrap());
+
+        // This internally uses a different from address
+        // so this must cause all outputs to be renderable
+        let view_fields = tx.review_fields(&ovk).unwrap();
+
+        assert_eq!(view_fields.len(), 11);
+
+        // Now set a different from address
+        // to ensure we filter out change address going
+        // to us
+        Transaction::set_from_address(TO_ADDR);
+
+        let view_fields = tx.review_fields(&ovk).unwrap();
+
+        assert_ne!(view_fields.len(), 11);
+    }
 }

tests_zemu/tests/common.ts

@@ -4,8 +4,8 @@ import { resolve } from 'path'
 
 export const APP_SEED = 'equip will roof matter pink blind book anxiety banner elbow sun young'
 
-const APP_PATH_S = resolve('../app/target/nanos/release/ironfish-dkg')
-const APP_PATH_X = resolve('../app/target/nanox/release/ironfish-dkg')
+// const APP_PATH_S = resolve('../app/target/nanos/release/ironfish-dkg')
+// const APP_PATH_X = resolve('../app/target/nanox/release/ironfish-dkg')
 const APP_PATH_SP = resolve('../app/target/nanosplus/release/ironfish-dkg')
 const APP_PATH_ST = resolve('../app/target/stax/release/ironfish-dkg')
 const APP_PATH_FL = resolve('../app/target/flex/release/ironfish-dkg')

tests_zemu/tests/dkg.test.ts

@@ -19,7 +19,7 @@ import { defaultOptions, models } from './common'
 import IronfishApp, { IronfishKeys } from '@zondax/ledger-ironfish'
 import { isValidPublicAddress, multisig, UnsignedTransaction, verifyTransactions } from '@ironfish/rust-nodejs'
 import { Transaction } from '@ironfish/sdk'
-import { buildTx, minimizeRound3Inputs, runMethod, startTextFn } from './utils'
+import { buildTx, IronfishKeySet, minimizeRound3Inputs, runMethod, startTextFn } from './utils'
 import aggregateSignatureShares = multisig.aggregateSignatureShares
 
 jest.setTimeout(4500000)
@@ -186,8 +186,6 @@ describe.each(models)('DKG', function (m) {
           publicPackages.push(result.publicPackage.toString('hex'))
         }
 
-        console.log('publicPackages ' + JSON.stringify(publicPackages, null, 2))
-
         for (let i = 0; i < participants; i++) {
           try {
             const result = await runMethod(m, globalSims, i, async (sim: Zemu, app: IronfishApp) => {
@@ -216,8 +214,6 @@ describe.each(models)('DKG', function (m) {
           }
         }
 
-        console.log('encryptedKeys ' + JSON.stringify(encryptedKeys, null, 2))
-
         // Generate keys from the multisig DKG process just finalized
         for (let i = 0; i < participants; i++) {
           const result = await runMethod(m, globalSims, i, async (_sim: Zemu, app: IronfishApp) => {
@@ -234,8 +230,6 @@ describe.each(models)('DKG', function (m) {
           pks.push(result.publicAddress.toString('hex'))
         }
 
-        console.log('publicAddresses ' + JSON.stringify(pks, null, 2))
-
         // Check that the public address generated on each participant for the multisig account is the same
         const pksMap = pks.reduce((acc: { [key: string]: boolean }, pk) => {
           if (!acc[pk]) acc[pk] = true
@@ -265,8 +259,6 @@ describe.each(models)('DKG', function (m) {
           })
         }
 
-        console.log('viewKeys ' + JSON.stringify(viewKeys, null, 2))
-
         // Generate view keys from the multisig DKG process just finalized
         for (let i = 0; i < participants; i++) {
           const result = await runMethod(m, globalSims, i, async (sim: Zemu, app: IronfishApp) => {
@@ -286,8 +278,6 @@ describe.each(models)('DKG', function (m) {
           })
         }
 
-        console.log('proofKeys ' + JSON.stringify(proofKeys, null, 2))
-
         // get identity from the multisig DKG process just finalized
         for (let i = 0; i < participants; i++) {
           const result = await runMethod(m, globalSims, i, async (sim: Zemu, app: IronfishApp) => {
@@ -305,7 +295,12 @@ describe.each(models)('DKG', function (m) {
 
         // Craft new tx, to get the tx hash and the public randomness
         // Pass those values to the following commands
-        const unsignedTxRaw = buildTx(pks[0], viewKeys[0], proofKeys[0])
+        let senderKey: IronfishKeySet = {
+          publicAddress: pks[0],
+          viewKey: viewKeys[0],
+          proofKey: proofKeys[0],
+        }
+        const unsignedTxRaw = buildTx(senderKey)
         const unsignedTx = new UnsignedTransaction(unsignedTxRaw)
         const serialized = unsignedTx.serialize()
 

tests_zemu/tests/failures.test.ts

@@ -1,6 +1,6 @@
 import { defaultOptions, identities, models, restoreKeysTestCases } from './common'
 import Zemu, { ButtonKind, isTouchDevice } from '@zondax/zemu'
-import { buildTx, runMethod, startTextFn } from './utils'
+import { buildTx, IronfishKeySet, runMethod, startTextFn } from './utils'
 import IronfishApp, { IronfishKeys } from '@zondax/ledger-ironfish'
 import { UnsignedTransaction } from '@ironfish/rust-nodejs'
 
@@ -86,7 +86,12 @@ describe.each(models)('wrong actions', function (m) {
           identities.push(identity.identity.toString('hex'))
         }
 
-        const unsignedTxRaw = buildTx(pubkey, viewKey, proofKey)
+        let senderKey: IronfishKeySet = {
+          publicAddress: pubkey,
+          viewKey: viewKey,
+          proofKey: proofKey,
+        }
+        const unsignedTxRaw = buildTx(senderKey)
         const unsignedTx = new UnsignedTransaction(unsignedTxRaw)
 
         const serialized = unsignedTx.serialize()
@@ -207,7 +212,12 @@ describe.each(models)('wrong actions', function (m) {
           identities.push(identity.identity.toString('hex'))
         }
 
-        const unsignedTxRaw = buildTx(pubkey, viewKey, proofKey)
+        let senderKey: IronfishKeySet = {
+          publicAddress: pubkey,
+          viewKey: viewKey,
+          proofKey: proofKey,
+        }
+        const unsignedTxRaw = buildTx(senderKey)
         const unsignedTx = new UnsignedTransaction(unsignedTxRaw)
 
         const serialized = unsignedTx.serialize()