Merge branch 'tiago/audit-todos' (#3234)

* origin/tiago/audit-todos:
  Changelog for #3234
  Audit TODOs in codebase

.changelog/unreleased/improvements/3234-audit-todos.md

@@ -0,0 +1,2 @@
+- Audit TODOs in the codebase.
+  ([\#3234](https://github.com/anoma/namada/pull/3234))

crates/apps_lib/src/cli.rs

@@ -3411,7 +3411,6 @@ pub mod args {
 
     #[derive(Clone, Debug)]
     pub struct LedgerDumpDb {
-        // TODO: allow to specify height
         pub block_height: Option<BlockHeight>,
         pub out_file_path: PathBuf,
         pub historic: bool,

crates/apps_lib/src/cli/api.rs

@@ -3,6 +3,7 @@ use namada::tendermint_rpc::HttpClient;
 use namada_sdk::error::Error;
 use namada_sdk::queries::Client;
 use namada_sdk::rpc::wait_until_node_is_synched;
+use tendermint_rpc::client::CompatMode;
 use tendermint_rpc::Url as TendermintUrl;
 
 /// Trait for clients that can be used with the CLI.
@@ -18,7 +19,11 @@ pub trait CliClient: Client + Sync {
 #[async_trait::async_trait(?Send)]
 impl CliClient for HttpClient {
     fn from_tendermint_address(address: &TendermintUrl) -> Self {
-        HttpClient::new(address.clone()).unwrap()
+        HttpClient::builder(address.clone().try_into().unwrap())
+            .compat_mode(CompatMode::V0_37)
+            .timeout(std::time::Duration::from_secs(30))
+            .build()
+            .unwrap()
     }
 
     async fn wait_until_node_is_synced(

crates/apps_lib/src/cli/client.rs

@@ -27,7 +27,6 @@ impl CliApi {
                 match cmd {
                     // Ledger cmds
                     Sub::TxCustom(TxCustom(args)) => {
-                        // TODO: too much boilerplate to setup client
                         let chain_ctx = ctx.borrow_mut_chain_or_exit();
                         let ledger_address =
                             chain_ctx.get(&args.tx.ledger_address);

crates/apps_lib/src/cli/utils.rs

@@ -20,7 +20,7 @@ use crate::cli::context::FromContext;
 
 /// Environment variable where Ethereum relayer private
 /// keys are stored.
-// TODO: remove this in favor of getting eth keys from
+// TODO(namada#2029): remove this in favor of getting eth keys from
 // namadaw, ledger, or something more secure
 #[cfg_attr(not(feature = "namada-eth-bridge"), allow(dead_code))]
 const RELAYER_KEY_ENV_VAR: &str = "NAMADA_RELAYER_KEY";

crates/apps_lib/src/client/rpc.rs

@@ -1046,13 +1046,20 @@ pub async fn query_bonded_stake<N: Namada>(
                 get_validator_stake(context.client(), epoch, &validator).await;
             match stake {
                 Some(stake) => {
-                    // TODO: show if it's in consensus set, below capacity, or
-                    // below threshold set
                     display_line!(
                         context.io(),
                         "Bonded stake of validator {validator}: {}",
                         stake.to_string_native()
+                    );
+                    query_and_print_validator_state(
+                        context,
+                        args::QueryValidatorState {
+                            query: args.query,
+                            validator,
+                            epoch: args.epoch,
+                        },
                     )
+                    .await;
                 }
                 None => {
                     display_line!(

crates/apps_lib/src/config/genesis.rs

@@ -598,7 +598,6 @@ pub mod tests {
         let (protocol_keypair, _eth_hot_bridge_keypair) =
             wallet::defaults::validator_keys();
 
-        // TODO: derive validator eth address from an eth keypair
         let eth_cold_gov_keypair: common::SecretKey =
             secp256k1::SigScheme::generate(&mut rng)
                 .try_to_sk()

crates/apps_lib/src/config/genesis/templates.rs

@@ -263,10 +263,10 @@ pub struct ChainParams<T: TemplateValidation> {
     /// Enable the native token transfer if it is true
     pub is_native_token_transferable: bool,
     /// Minimum number of blocks per epoch.
-    // TODO: u64 only works with values up to i64::MAX with toml-rs!
+    // NB: u64 only works with values up to i64::MAX with toml-rs!
     pub min_num_of_blocks: u64,
     /// Maximum duration per block (in seconds).
-    // TODO: this is i64 because datetime wants it
+    // NB: this is i64 because datetime wants it
     pub max_expected_time_per_block: i64,
     /// Max payload size, in bytes, for a tx batch proposal.
     ///

crates/apps_lib/src/config/genesis/transactions.rs

@@ -591,7 +591,7 @@ pub type SignedBondTx<T> = Signed<BondTx<T>>;
 pub struct ValidatorAccountTx<PK: Ord> {
     /// The address of the validator.
     pub address: StringEncoded<EstablishedAddress>,
-    // TODO: remove the vp field
+    // TODO(namada#2554): remove the vp field
     pub vp: String,
     /// Commission rate charged on rewards for delegators (bounded inside
     /// 0-1)

crates/apps_lib/src/config/mod.rs

@@ -364,9 +364,8 @@ And this is correct
     }
 "#;
 
-// TODO: Replaced `block_sync` and `blocksync`
-// with `fast_sync` and `fastsync`
-// due to https://github.com/informalsystems/tendermint-rs/issues/1368
+// TODO(informalsystems/tendermint-rs#1368): Replaced
+// `block_sync` and `blocksync` with `fast_sync` and `fastsync`
 pub const DEFAULT_COMETBFT_CONFIG: &str = r#"
 
 # This is a TOML config file.

crates/apps_lib/src/lib.rs

@@ -28,7 +28,7 @@ pub mod wasm_loader;
 pub use std;
 
 pub mod facade {
-    // TODO: re-import v0_37 only
+    // TODO(namada#3248): only re-export v037 `tendermint-rs`
     pub use namada::{tendermint, tendermint_proto, tendermint_rpc};
     pub use tendermint_config;
 }

crates/apps_lib/src/wallet/mod.rs

@@ -228,7 +228,7 @@ where
         .map(|pk| {
             let pkh = PublicKeyHash::from(&pk);
             wallet
-                // TODO: optionally encrypt validator keys
+                // TODO(namada#3251): optionally encrypt validator keys
                 .find_key_by_pkh(&pkh, None)
                 .ok()
                 .or_else(|| wallet.get_validator_data().map(extract_key))

crates/benches/native_vps.rs

@@ -241,7 +241,9 @@ fn governance(c: &mut Criterion) {
     group.finish();
 }
 
-// TODO: uncomment when SlashFund internal address is brought back
+// TODO(namada#2984): uncomment when SlashFund internal
+// address is brought back
+//
 // fn slash_fund(c: &mut Criterion) {
 //      let mut group = c.benchmark_group("vp_slash_fund");
 

crates/controller/src/lib.rs

@@ -101,8 +101,6 @@ impl PDController {
         max_inflation.to_uint().ok_or(Error::MaxInflationOverflow)
     }
 
-    // TODO: could possibly use I256 instead of Dec here (need to account for
-    // negative vals)
     fn compute_inflation_aux(&self, control: Dec) -> Result<Uint, Error> {
         let last_inflation_amount = Dec::try_from(self.last_inflation_amount)?;
         let new_inflation_amount = checked!(last_inflation_amount + control)?;

crates/core/src/address.rs

@@ -848,8 +848,8 @@ pub mod testing {
             Just(InternalAddress::Governance),
             Just(InternalAddress::EthBridge),
             Just(InternalAddress::EthBridgePool),
-            Just(arb_erc20()),
-            Just(arb_nut()),
+            arb_erc20(),
+            arb_nut(),
             Just(InternalAddress::Multitoken),
             Just(InternalAddress::Pgf),
             Just(InternalAddress::Masp),
@@ -879,16 +879,16 @@ pub mod testing {
         })
     }
 
-    fn arb_erc20() -> InternalAddress {
-        use crate::ethereum_events::testing::arbitrary_eth_address;
-        // TODO: generate random erc20 addr data
-        InternalAddress::Erc20(arbitrary_eth_address())
+    fn arb_erc20() -> impl Strategy<Value = InternalAddress> {
+        proptest::array::uniform20(proptest::num::u8::ANY).prop_map(|addr| {
+            InternalAddress::Erc20(crate::ethereum_events::EthAddress(addr))
+        })
     }
 
-    fn arb_nut() -> InternalAddress {
-        use crate::ethereum_events::testing::arbitrary_eth_address;
-        // TODO: generate random erc20 addr data
-        InternalAddress::Nut(arbitrary_eth_address())
+    fn arb_nut() -> impl Strategy<Value = InternalAddress> {
+        proptest::array::uniform20(proptest::num::u8::ANY).prop_map(|addr| {
+            InternalAddress::Nut(crate::ethereum_events::EthAddress(addr))
+        })
     }
 
     /// NAM token address for testing
@@ -935,7 +935,6 @@ pub mod testing {
 
     /// Imaginary eth address for testing
     pub const fn wnam() -> EthAddress {
-        // TODO: Replace this with the real wNam ERC20 address once it exists
         // "DEADBEEF DEADBEEF DEADBEEF DEADBEEF DEADBEEF"
         EthAddress([
             222, 173, 190, 239, 222, 173, 190, 239, 222, 173, 190, 239, 222,

crates/core/src/chain.rs

@@ -95,7 +95,10 @@ impl<'de> Deserialize<'de> for ProposalBytes {
             }
 
             // NOTE: this is only needed because of a bug in the toml parser
-            // https://github.com/toml-rs/toml-rs/issues/256
+            // - https://github.com/toml-rs/toml-rs/issues/256
+            // - https://github.com/toml-rs/toml/issues/512
+            //
+            // TODO(namada#3243): switch to `toml_edit` for TOML parsing
             fn visit_i64<E>(self, size: i64) -> Result<Self::Value, E>
             where
                 E: serde::de::Error,

crates/core/src/dec.rs

@@ -61,15 +61,21 @@ impl Dec {
     /// means that any fractional part of the result that exceeds
     /// [`POS_DECIMAL_PRECISION`] is discarded.
     ///
+    /// ## Breakdown of the algorithm
+    ///
     /// The division is performed in the following way:
+    ///
     /// 1. The absolute values of the numerator and denominator are used for the
-    /// division. 2. The result is calculated to a fixed precision defined
-    /// by [`POS_DECIMAL_PRECISION`]. 3. If either the numerator or
-    /// denominator (but not both) is negative, the result is negated. 4. If
-    /// the division is impossible (e.g., division by zero or overflow), `None`
-    /// is returned.
+    /// division.
+    /// 2. The result is calculated to a fixed precision defined
+    /// by [`POS_DECIMAL_PRECISION`].
+    /// 3. If either the numerator or
+    /// denominator (but not both) is negative, the result is negated.
+    /// 4. If the division is impossible (e.g., division by zero or overflow),
+    ///    `None` is returned.
+    ///
+    /// ## Example
     ///
-    /// Example:
     /// ```
     /// use namada_core::dec::Dec;
     ///
@@ -79,11 +85,11 @@ impl Dec {
     /// assert_eq!(result, Dec::new(15, 1).unwrap()); // Result is 1.5 truncated to 1 decimal place
     /// ```
     ///
-    /// # Arguments
+    /// ## Arguments
     ///
     /// * `rhs`: The right-hand side `Dec` value for the division.
     ///
-    /// # Returns
+    /// ## Returns
     ///
     /// An `Option<Dec>` which is `Some` with the result if the division is
     /// successful, or `None` if the division cannot be performed.

crates/core/src/eth_abi.rs

@@ -118,7 +118,6 @@ impl<const N: usize> Encode<N> for AbiEncode<N> {
     }
 }
 
-// TODO: test signatures here once we merge secp keys
 #[cfg(test)]
 mod tests {
     use std::str::FromStr;

crates/core/src/eth_bridge_pool.rs

@@ -301,7 +301,7 @@ impl From<&PendingTransfer> for TransferToEthereumEvent {
 
 impl Encode<6> for PendingTransfer {
     fn tokenize(&self) -> [Token; 6] {
-        // TODO: This version should be looked up from storage
+        // TODO(namada#249): This version should be looked up from storage
         let version = Token::Uint(VERSION.into());
         let namespace = Token::String(NAMESPACE.into());
         let from = Token::Address(self.transfer.asset.0.into());
@@ -314,7 +314,7 @@ impl Encode<6> for PendingTransfer {
 
 impl Encode<6> for TransferToEthereumEvent {
     fn tokenize(&self) -> [Token; 6] {
-        // TODO: This version should be looked up from storage
+        // TODO(namada#249): This version should be looked up from storage
         let version = Token::Uint(VERSION.into());
         let namespace = Token::String(NAMESPACE.into());
         let from = Token::Address(self.asset.0.into());

crates/core/src/ethereum_events.rs

@@ -38,6 +38,7 @@ use crate::token::Amount;
     BorshDeserializer,
     BorshSchema,
 )]
+#[repr(align(32))]
 pub struct Uint(pub [u64; 4]);
 
 impl PartialOrd for Uint {
@@ -256,42 +257,36 @@ impl From<TransfersToNamada> for EthereumEvent {
     BorshDeserializer,
     BorshSchema,
 )]
+// NOTE: Avoid changing the order of the elements in this struct,
+// to maintain compatibility between Namada versions.
 pub enum EthereumEvent {
     /// Event transferring batches of ether or Ethereum based ERC20 tokens
     /// from Ethereum to wrapped assets on Namada
     TransfersToNamada {
         /// Monotonically increasing nonce
-        #[allow(dead_code)]
         nonce: Uint,
         /// The batch of transfers
-        #[allow(dead_code)]
         transfers: Vec<TransferToNamada>,
     },
     /// A confirmation event that a batch of transfers have been made
     /// from Namada to Ethereum
     TransfersToEthereum {
         /// Monotonically increasing nonce
-        #[allow(dead_code)]
         nonce: Uint,
         /// The batch of transfers
-        #[allow(dead_code)]
         transfers: Vec<TransferToEthereum>,
         /// The Namada address that receives the gas fees
         /// for relaying a batch of transfers
-        #[allow(dead_code)]
         relayer: Address,
     },
     /// Event indication that the validator set has been updated
     /// in the governance contract
     ValidatorSetUpdate {
         /// Monotonically increasing nonce
-        #[allow(dead_code)]
         nonce: Uint,
         /// Hash of the validators in the bridge contract
-        #[allow(dead_code)]
         bridge_validator_hash: KeccakHash,
         /// Hash of the validators in the governance contract
-        #[allow(dead_code)]
         governance_validator_hash: KeccakHash,
     },
 }