Apply suggestions from code review

Co-authored-by: str4d <thestr4d@gmail.com>

qa/rpc-tests/wallet_listreceived.py

@@ -459,6 +459,9 @@ class ListReceivedTest (BitcoinTestFramework):
         self.nodes[0].sendtoaddress(taddr, 4.0)
         self.generate_and_sync(height+2)
 
+        acct_node0 = self.nodes[0].z_getnewaccount()['account']
+        ua_node0 = self.nodes[0].z_getaddressforaccount(acct_node0, ['sapling', 'orchard'])['unifiedaddress']
+
         opid = self.nodes[1].z_sendmany(taddr, [
             {'address': uao, 'amount': 1, 'memo': my_memo},
             {'address': uaso, 'amount': 2},
@@ -497,7 +500,8 @@ class ListReceivedTest (BitcoinTestFramework):
         self.generate_and_sync(height+3)
 
         opid = self.nodes[1].z_sendmany(uao, [
-            {'address': uaso, 'amount': Decimal('0.4')}
+            {'address': uaso, 'amount': Decimal('0.4')},
+            {'address': ua_node0, 'amount': Decimal('0.2')}
         ])
         txid1 = wait_and_assert_operationid_status(self.nodes[1], opid)
         self.sync_all()
@@ -506,7 +510,7 @@ class ListReceivedTest (BitcoinTestFramework):
 
         assert_equal(pt['txid'], txid1)
         assert_equal(len(pt['spends']), 1) # one spend we can see
-        assert_equal(len(pt['outputs']), 2) # one output + one change output we can see
+        assert_equal(len(pt['outputs']), 3) # one output + one change output we can see
 
         spends = pt['spends']
         assert_equal(spends[0]['type'], 'orchard')
@@ -519,20 +523,27 @@ class ListReceivedTest (BitcoinTestFramework):
 
         outputs = sorted(pt['outputs'], key=lambda x: x['valueZat'])
         assert_equal(outputs[0]['type'], 'orchard')
-        assert_equal(outputs[0]['address'], uaso)
+        assert_equal(outputs[0]['address'], ua_node0)
-        assert_equal(outputs[0]['value'], Decimal('0.4'))
+        assert_equal(outputs[0]['value'], Decimal('0.2'))
-        assert_equal(outputs[0]['valueZat'], 40000000)
+        assert_equal(outputs[0]['valueZat'], 20000000)
-        assert_equal(outputs[0]['outgoing'], False)
+        assert_equal(outputs[0]['outgoing'], True)
         assert_equal(outputs[0]['memo'], no_memo)
 
         assert_equal(outputs[1]['type'], 'orchard')
-        # TODO: scrub change addresses
+        assert_equal(outputs[1]['address'], uaso)
-        #assert_equal(outputs[1]['address'], '<change>')
+        assert_equal(outputs[1]['value'], Decimal('0.4'))
-        assert_equal(outputs[1]['value'], Decimal('0.59999'))
+        assert_equal(outputs[1]['valueZat'], 40000000)
-        assert_equal(outputs[1]['valueZat'], 59999000)
         assert_equal(outputs[1]['outgoing'], False)
         assert_equal(outputs[1]['memo'], no_memo)
 
+        assert_equal(outputs[2]['type'], 'orchard')
+        # TODO: scrub change addresses
+        #assert_equal(outputs[2]['address'], '<change>')
+        assert_equal(outputs[2]['value'], Decimal('0.39998'))
+        assert_equal(outputs[2]['valueZat'], 39998000)
+        assert_equal(outputs[2]['outgoing'], False)
+        assert_equal(outputs[2]['memo'], no_memo)
+
     def run_test(self):
         self.test_received_sprout(200)
         self.test_received_sapling(214)

src/rust/include/rust/orchard/wallet.h

@@ -294,21 +294,23 @@ typedef void (*push_spend_t)(void* callbackReceiver, const RawOrchardActionSpend
 typedef void (*push_output_t)(void* callbackReceiver, const RawOrchardActionOutput data);
 
 /**
- * Trial-decrypt the specfied Orchard bundle, and
+ * Trial-decrypts the specfied Orchard bundle, and uses the provided callbacks to pass
- * uses the provided callback to push RawOrchardActionData values corresponding to the
+ * `RawOrchardActionSpend` and `RawOrchardActionOutput` values (corresponding to the
- * actions of that bundle on to the provided result vector.  Note that the push_cb callback can perform any
+ * actions of that bundle) to the provided result receiver.
- * necessary conversion from a RawOrchardActionData value in addition to modifying the
- * provided result vector.
  *
- * The following pointers must be freed by the caller:
+ * Note that the callbacks can perform any necessary conversion from a
- * - `RawOrchardActionData::spend` must be freed using `orchard_action_spend_free`
+ * `RawOrchardActionSpend` or `RawOrchardActionOutput` value in addition to modifying the
- * - `RawOrchardActionData::output` must be freed using `orchard_action_output_free`
+ * provided result receiver.
- * - `RawOrchardActionData::output::addr` must be freed using `orchard_address_free`
+ *
+ * `raw_ovks` must be a pointer to an array of `unsigned char[32]`.
+ *
+ * The `addr` pointer on each `RawOrchardActionOutput` value must be freed using
+ * `orchard_address_free`.
  */
 bool orchard_wallet_get_txdata(
         const OrchardWalletPtr* wallet,
         const OrchardBundlePtr* bundle,
-        const unsigned char*,
+        const unsigned char* raw_ovks,
         size_t raw_ovks_len,
         void* callbackReceiver,
         push_spend_t push_spend_cb,

src/rust/src/wallet.rs

@@ -1041,7 +1041,7 @@ pub extern "C" fn orchard_wallet_get_txdata(
         .map(|k| OutgoingViewingKey::from(*k))
         .collect();
     if let Some(bundle) = unsafe { bundle.as_ref() } {
-        let keys = wallet
+        let ivks = wallet
             .key_store
             .viewing_keys
             .keys()
@@ -1049,7 +1049,7 @@ pub extern "C" fn orchard_wallet_get_txdata(
             .collect::<Vec<_>>();
 
         let incoming: BTreeMap<usize, (Note, Address, [u8; 512])> = bundle
-            .decrypt_outputs_with_keys(&keys)
+            .decrypt_outputs_with_keys(&ivks)
             .into_iter()
             .map(|(idx, _, note, addr, memo)| (idx, (note, addr, memo)))
             .collect();
@@ -1077,22 +1077,18 @@ pub extern "C" fn orchard_wallet_get_txdata(
             }) {
                 unsafe { (spend_push_cb.unwrap())(callback_receiver, spend) };
             }
-            if let Some((note, addr, memo)) = incoming.get(&idx) {
+
+            if let Some(((note, addr, memo), is_outgoing)) = incoming
+                .get(&idx)
+                .map(|n| (n, false))
+                .or_else(|| outgoing.get(&idx).map(|n| (n, true)))
+            {
                 let output = FFIActionOutput {
                     action_idx: idx as u32,
                     recipient: Box::into_raw(Box::new(*addr)),
                     value: note.value().inner() as i64,
                     memo: *memo,
-                    is_outgoing: false,
+                    is_outgoing,
-                };
-                unsafe { (output_push_cb.unwrap())(callback_receiver, output) };
-            } else if let Some((note, addr, memo)) = outgoing.get(&idx) {
-                let output = FFIActionOutput {
-                    action_idx: idx as u32,
-                    recipient: Box::into_raw(Box::new(*addr)),
-                    value: note.value().inner() as i64,
-                    memo: *memo,
-                    is_outgoing: true,
                 };
                 unsafe { (output_push_cb.unwrap())(callback_receiver, output) };
             }

src/wallet/rpcwallet.cpp

@@ -4288,7 +4288,7 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp)
     }
 
     std::vector<uint256> ovksVector(ovks.begin(), ovks.end());
-    OrchardActions orchardActions = pwalletMain->GetOrchardWallet().GetTxActions(wtx, ovksVector);
+    OrchardActions orchardActions = wtx.RecoverOrchardActions(ovksVector);
 
     // Orchard spends
     for (auto & pair  : orchardActions.GetSpends()) {
@@ -4303,9 +4303,9 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp)
 
         UniValue entry(UniValue::VOBJ);
         entry.pushKV("type", ADDR_TYPE_ORCHARD);
-        entry.pushKV("spend", (int) actionIdx);
+        entry.pushKV("action", (int) actionIdx);
         entry.pushKV("txidPrev", outpoint.hash.GetHex());
-        entry.pushKV("outputPrev", (int) outpoint.n);
+        entry.pushKV("actionPrev", (int) outpoint.n);
         auto ua = pwalletMain->FindUnifiedAddressByReceiver(receivedAt);
         assert(ua.has_value());
         std::string addrStr = keyIO.EncodePaymentAddress(ua.value());
@@ -4329,7 +4329,7 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp)
 
         UniValue entry(UniValue::VOBJ);
         entry.pushKV("type", ADDR_TYPE_ORCHARD);
-        entry.pushKV("output", (int) actionIdx);
+        entry.pushKV("action", (int) actionIdx);
         entry.pushKV("outgoing", orchardActionOutput.IsOutgoing());
         entry.pushKV("address", address);
         entry.pushKV("value", ValueFromAmount(noteValue));

src/wallet/wallet.cpp

@@ -3933,6 +3933,12 @@ std::optional<std::pair<
     return std::nullopt;
 }
 
+OrchardActions CWalletTx::RecoverOrchardActions(const std::vector<uint256>& ovks) const
+{
+    return pwallet->orchardWallet.GetTxActions(*this, ovks);
+}
+
+
 int64_t CWalletTx::GetTxTime() const
 {
     int64_t n = nTimeSmart;

src/wallet/wallet.h

@@ -632,6 +632,7 @@ public:
     std::optional<std::pair<
         libzcash::SaplingNotePlaintext,
         libzcash::SaplingPaymentAddress>> RecoverSaplingNoteWithoutLeadByteCheck(SaplingOutPoint op, std::set<uint256>& ovks) const;
+    OrchardActions RecoverOrchardActions(const std::vector<uint256>& ovks) const;
 
     //! filter decides which addresses will count towards the debit
     CAmount GetDebit(const isminefilter& filter) const;
@@ -976,6 +977,8 @@ public:
 class CWallet : public CCryptoKeyStore, public CValidationInterface
 {
 private:
+    friend class CWalletTx;
+
     /**
      * Select a set of coins such that nValueRet >= nTargetValue and at least
      * all coins from coinControl are selected; Never select unconfirmed coins
@@ -1190,8 +1193,6 @@ public:
         pwalletdbEncryption = NULL;
     }
 
-    OrchardWallet& GetOrchardWallet() { return orchardWallet; }
-
     void SetNull(const CChainParams& params)
     {
         nWalletVersion = FEATURE_BASE;