fix logic error in get_latest_balance_height()

Addresses #56 (comment)

1. rewrite get_latest_balance_height() to find latest spent utxo
   anywhere in list of monitored_utxos.
2. change type of Confirmations field from u64 to BlockHeight
   in server and Dashboard.

src/bin/dashboard_src/overview_screen.rs

@@ -11,6 +11,7 @@ use chrono::DateTime;
 use itertools::Itertools;
 use neptune_core::config_models::network::Network;
 use neptune_core::models::blockchain::block::block_header::BlockHeader;
+use neptune_core::models::blockchain::block::block_height::BlockHeight;
 use neptune_core::models::blockchain::shared::Hash;
 use neptune_core::models::blockchain::transaction::amount::Amount;
 use neptune_core::rpc_server::RPCClient;
@@ -31,7 +32,7 @@ use super::screen::Screen;
 #[derive(Debug, Clone)]
 pub struct OverviewData {
     synced_balance: Option<Amount>,
-    confirmations: Option<u64>,
+    confirmations: Option<BlockHeight>,
     synchronization_percentage: Option<f64>,
 
     network: Network,
@@ -91,7 +92,7 @@ impl OverviewData {
     pub fn test() -> Self {
         OverviewData {
             synced_balance: Some(Amount::zero()),
-            confirmations: Some(17),
+            confirmations: Some(17.into()),
             synchronization_percentage: Some(99.5),
 
             listen_address: None,

src/models/blockchain/transaction/amount.rs

@@ -43,6 +43,16 @@ pub enum Sign {
     Negative,
 }
 
+impl Display for Sign {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let s = match *self {
+            Self::NonNegative => "",
+            Self::Negative => "-",
+        };
+        write!(f, "{}", s)
+    }
+}
+
 pub const AMOUNT_SIZE_FOR_U32: usize = 4;
 
 #[derive(Clone, Copy, Debug, Serialize, Deserialize, Eq, BFieldCodec)]

src/models/state/wallet/wallet_state.rs

@@ -860,22 +860,59 @@ impl WalletState {
 
     /// Retrieve block height of last change to wallet balance.
     ///
+    /// note: this fn could be implemented as:
+    ///   1. get_balance_history()
+    ///   2. sort by height
+    ///   3. return height of last entry.
+    ///
+    /// this implementation is a bit more efficient as it avoids
+    ///   the sort and some minor work looking up amount and confirmation
+    ///   height of each utxo.
+    ///
+    /// Presently this is o(n) with the number of monitored utxos.
+    /// if storage could keep track of latest spend utxo, then this could
+    /// be o(1).
+    ///
     /// todo: ignore abandoned/unsynced utxo.  
     ///       also an issue for get_balance_history().
     /// see: https://github.com/Neptune-Crypto/neptune-core/issues/28
     pub async fn get_latest_balance_height(&self) -> Option<BlockHeight> {
-        let db_lock = self.wallet_db.lock().await;
-        let len = db_lock.monitored_utxos.len();
+        let monitored_utxos = self.wallet_db.lock().await.monitored_utxos.clone();
+        let len = monitored_utxos.len();
+
         if len > 0 {
-            let utxo = db_lock.monitored_utxos.get(len - 1);
-            if let Some((.., confirmation_height)) = utxo.confirmed_in_block {
+            let mut latest: BlockHeight = 0.into();
+
+            // note: would be nicer to use an iterator to find max spending_height.
+            // perhaps someday: https://github.com/Neptune-Crypto/twenty-first/issues/139
+            for i in 0..len {
+                // latest change to balance could be a spend utxo, and it could be anywhere
+                // in the monitored_utxos list, so we must check each entry in list.
+                let utxo = monitored_utxos.get(i);
                 if let Some((.., spending_height)) = utxo.spent_in_block {
-                    return Some(spending_height);
+                    if spending_height > latest {
+                        latest = spending_height;
+                    }
+                }
+
+                // if latest change is an incoming utxo, we can save some work by checking only
+                // the last entry because monitored_utxos is already ordered by
+                // confirmation_height ascending
+                if i == len - 1 {
+                    if let Some((.., confirmation_height)) = utxo.confirmed_in_block {
+                        if confirmation_height > latest {
+                            latest = confirmation_height;
+                        }
+                    }
                 }
-                return Some(confirmation_height);
             }
+            assert!(
+                latest > 0.into() || (len == 1 && monitored_utxos.get(0).spent_in_block.is_none())
+            );
+            Some(latest)
+        } else {
+            None
         }
-        None
     }
 }
 

src/rpc_server.rs

@@ -41,7 +41,7 @@ pub struct DashBoardOverviewDataFromClient {
 
     // # of confirmations since last wallet balance change.
     // `None` indicates that wallet balance has never changed.
-    pub confirmations: Option<u64>,
+    pub confirmations: Option<BlockHeight>,
 }
 
 #[tarpc::service]
@@ -58,10 +58,10 @@ pub trait RPC {
 
     /// Returns the number of blocks (confirmations) since wallet balance last changed.
     ///
-    /// returns Option<u64>
+    /// returns Option<BlockHeight>
     ///
     /// return value will be None if wallet has not received any incoming funds.
-    async fn get_confirmations() -> Option<u64>;
+    async fn get_confirmations() -> Option<BlockHeight>;
 
     /// Returns info about the peers we are connected to
     async fn get_peer_info() -> Vec<PeerInfo>;
@@ -143,7 +143,7 @@ impl RPC for NeptuneRPCServer {
     type GetNetworkFut = Ready<Network>;
     type GetListenAddressForPeersFut = Ready<Option<SocketAddr>>;
     type BlockHeightFut = Ready<BlockHeight>;
-    type GetConfirmationsFut = Ready<Option<u64>>;
+    type GetConfirmationsFut = Ready<Option<BlockHeight>>;
     type GetPeerInfoFut = Ready<Vec<PeerInfo>>;
     type HeadFut = Ready<Digest>;
     type HeadsFut = Ready<Vec<Digest>>;
@@ -197,11 +197,12 @@ impl RPC for NeptuneRPCServer {
                     executor::block_on(self.state.chain.light_state.get_latest_block_header());
 
                 assert!(tip_block_header.height >= latest_balance_height);
-                assert!(tip_block_header.height - latest_balance_height <= u64::MAX.into());
 
                 // subtract latest balance height from chain tip.
-                // the cast to u64 is safe given we passed the above asserts.
-                let confirmations: u64 = (tip_block_header.height - latest_balance_height) as u64;
+                // note: BlockHeight is u64 internally and BlockHeight::sub() returns i128.
+                //       The subtraction and cast is safe given we passed the above assert.
+                let confirmations: BlockHeight =
+                    ((tip_block_header.height - latest_balance_height) as u64).into();
                 future::ready(Some(confirmations))
             }
             None => future::ready(None),