diff --git a/.changelog/unreleased/improvements/3234-audit-todos.md b/.changelog/unreleased/improvements/3234-audit-todos.md new file mode 100644 index 00000000000..9dbe8c73fd2 --- /dev/null +++ b/.changelog/unreleased/improvements/3234-audit-todos.md @@ -0,0 +1,2 @@ +- Audit TODOs in the codebase. + ([\#3234](https://github.com/anoma/namada/pull/3234)) \ No newline at end of file diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 7b855547ffe..bef722d0428 100644 --- a/crates/apps_lib/src/cli.rs +++ b/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, pub out_file_path: PathBuf, pub historic: bool, diff --git a/crates/apps_lib/src/cli/api.rs b/crates/apps_lib/src/cli/api.rs index bd2a99703e6..2dd8d9c9c83 100644 --- a/crates/apps_lib/src/cli/api.rs +++ b/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( diff --git a/crates/apps_lib/src/cli/client.rs b/crates/apps_lib/src/cli/client.rs index 222653d4061..03f69c3f3e6 100644 --- a/crates/apps_lib/src/cli/client.rs +++ b/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); diff --git a/crates/apps_lib/src/cli/utils.rs b/crates/apps_lib/src/cli/utils.rs index 23dcb65f0ed..3c5280e2aaa 100644 --- a/crates/apps_lib/src/cli/utils.rs +++ b/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"; diff --git a/crates/apps_lib/src/client/rpc.rs b/crates/apps_lib/src/client/rpc.rs index 55293e8ceb6..ac8f98d3b28 100644 --- a/crates/apps_lib/src/client/rpc.rs +++ b/crates/apps_lib/src/client/rpc.rs @@ -1046,13 +1046,20 @@ pub async fn query_bonded_stake( 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!( diff --git a/crates/apps_lib/src/config/genesis.rs b/crates/apps_lib/src/config/genesis.rs index 08db754c1b3..ae439e784ad 100644 --- a/crates/apps_lib/src/config/genesis.rs +++ b/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() diff --git a/crates/apps_lib/src/config/genesis/templates.rs b/crates/apps_lib/src/config/genesis/templates.rs index f76c79b1c00..7a3ae4764c1 100644 --- a/crates/apps_lib/src/config/genesis/templates.rs +++ b/crates/apps_lib/src/config/genesis/templates.rs @@ -263,10 +263,10 @@ pub struct ChainParams { /// 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. /// diff --git a/crates/apps_lib/src/config/genesis/transactions.rs b/crates/apps_lib/src/config/genesis/transactions.rs index b9f921d7edf..b7015a9032a 100644 --- a/crates/apps_lib/src/config/genesis/transactions.rs +++ b/crates/apps_lib/src/config/genesis/transactions.rs @@ -591,7 +591,7 @@ pub type SignedBondTx = Signed>; pub struct ValidatorAccountTx { /// The address of the validator. pub address: StringEncoded, - // 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) diff --git a/crates/apps_lib/src/config/mod.rs b/crates/apps_lib/src/config/mod.rs index 018783debc5..d5041b184b9 100644 --- a/crates/apps_lib/src/config/mod.rs +++ b/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. diff --git a/crates/apps_lib/src/lib.rs b/crates/apps_lib/src/lib.rs index 0240b28ad80..58cdcbf701c 100644 --- a/crates/apps_lib/src/lib.rs +++ b/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; } diff --git a/crates/apps_lib/src/wallet/mod.rs b/crates/apps_lib/src/wallet/mod.rs index b35575e25b0..70cb3c78691 100644 --- a/crates/apps_lib/src/wallet/mod.rs +++ b/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)) diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index 0a0d08a1dd4..0ad8a637b5c 100644 --- a/crates/benches/native_vps.rs +++ b/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"); diff --git a/crates/controller/src/lib.rs b/crates/controller/src/lib.rs index 200512db50f..8dd55738080 100644 --- a/crates/controller/src/lib.rs +++ b/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 { let last_inflation_amount = Dec::try_from(self.last_inflation_amount)?; let new_inflation_amount = checked!(last_inflation_amount + control)?; diff --git a/crates/core/src/address.rs b/crates/core/src/address.rs index c28eee1bac0..bdda98c56e1 100644 --- a/crates/core/src/address.rs +++ b/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 { + 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 { + 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, diff --git a/crates/core/src/chain.rs b/crates/core/src/chain.rs index 3b3c1c5eed9..e6c75e539b5 100644 --- a/crates/core/src/chain.rs +++ b/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(self, size: i64) -> Result where E: serde::de::Error, diff --git a/crates/core/src/dec.rs b/crates/core/src/dec.rs index 53892b241ab..f6c6484e986 100644 --- a/crates/core/src/dec.rs +++ b/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` which is `Some` with the result if the division is /// successful, or `None` if the division cannot be performed. diff --git a/crates/core/src/eth_abi.rs b/crates/core/src/eth_abi.rs index 3d460a25ae1..f516639d5c6 100644 --- a/crates/core/src/eth_abi.rs +++ b/crates/core/src/eth_abi.rs @@ -118,7 +118,6 @@ impl Encode for AbiEncode { } } -// TODO: test signatures here once we merge secp keys #[cfg(test)] mod tests { use std::str::FromStr; diff --git a/crates/core/src/eth_bridge_pool.rs b/crates/core/src/eth_bridge_pool.rs index 69cb0291ada..854aac4df1a 100644 --- a/crates/core/src/eth_bridge_pool.rs +++ b/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()); diff --git a/crates/core/src/ethereum_events.rs b/crates/core/src/ethereum_events.rs index 3fe455494b6..0a8de94ba85 100644 --- a/crates/core/src/ethereum_events.rs +++ b/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 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, }, /// 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, /// 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, }, } diff --git a/crates/core/src/key/secp256k1.rs b/crates/core/src/key/secp256k1.rs index b8534338825..441d30beac5 100644 --- a/crates/core/src/key/secp256k1.rs +++ b/crates/core/src/key/secp256k1.rs @@ -316,76 +316,62 @@ impl super::Signature for Signature { } } -// Would ideally like Serialize, Deserialize to be implemented in k256, -// may try to do so and merge upstream in the future. +// NB: `RecoveryId` does not implement `serde` traits, so we can't derive them +// either impl Serialize for Signature { fn serialize(&self, serializer: S) -> Result where S: Serializer, { - let arr = self.0.to_bytes(); - // TODO: implement the line below, currently cannot support [u8; 64] - // serde::Serialize::serialize(&arr, serializer) + let mut seq = serializer.serialize_tuple(2)?; - // There is no way the bytes len + 1 will overflow - #[allow(clippy::arithmetic_side_effects)] - let mut seq = serializer.serialize_tuple(arr.len() + 1)?; - for elem in &arr[..] { - seq.serialize_element(elem)?; - } + seq.serialize_element(&self.0)?; seq.serialize_element(&self.1.to_byte())?; + seq.end() } } +// NB: `RecoveryId` does not implement `serde` traits, so we can't derive them +// either impl<'de> Deserialize<'de> for Signature { fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - struct ByteArrayVisitor; + struct SigVisitor; - impl<'de> Visitor<'de> for ByteArrayVisitor { - type Value = [u8; SIGNATURE_SIZE]; + impl<'de> Visitor<'de> for SigVisitor { + type Value = Signature; fn expecting( &self, formatter: &mut fmt::Formatter<'_>, ) -> fmt::Result { - formatter.write_str(&format!( - "an array of length {}", - SIGNATURE_SIZE, - )) + formatter.write_str("a secp256k1 signature") } - fn visit_seq( - self, - mut seq: A, - ) -> Result<[u8; SIGNATURE_SIZE], A::Error> + fn visit_seq(self, mut seq: A) -> Result where A: SeqAccess<'de>, { - let mut arr = [0u8; SIGNATURE_SIZE]; - #[allow(clippy::needless_range_loop)] - for i in 0..SIGNATURE_SIZE { - arr[i] = seq - .next_element()? - .ok_or_else(|| Error::invalid_length(i, &self))?; - } - Ok(arr) + let inner_sig: k256::ecdsa::Signature = seq + .next_element()? + .ok_or_else(|| Error::custom("Missing inner signature"))?; + let recovery_id = seq.next_element()?.ok_or_else(|| { + Error::custom("Missing signature recovery id") + })?; + + let recovery_id = RecoveryId::from_byte(recovery_id) + .ok_or_else(|| { + Error::custom("Invalid signature recovery id") + })?; + + Ok(Signature(inner_sig, recovery_id)) } } - let arr_res = - deserializer.deserialize_tuple(SIGNATURE_SIZE, ByteArrayVisitor)?; - let sig_array: [u8; 64] = arr_res[..64].try_into().unwrap(); - let sig = k256::ecdsa::Signature::from_slice(&sig_array) - .map_err(D::Error::custom); - Ok(Signature( - sig.unwrap(), - RecoveryId::from_byte(arr_res[64]) - .ok_or_else(|| Error::custom("Invalid recovery byte"))?, - )) + deserializer.deserialize_tuple(2, SigVisitor) } } diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 7018db9035e..522db30aa5e 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -21,6 +21,7 @@ pub mod arith; pub mod bytes; pub mod hints; +// TODO(namada#3248): only re-export v037 `tendermint-rs` pub use {masp_primitives, tendermint, tendermint_proto}; /// Borsh binary encoding (re-exported) from official crate with custom ext. pub mod borsh { diff --git a/crates/core/src/uint.rs b/crates/core/src/uint.rs index 6990ea052a5..a4f3f22254c 100644 --- a/crates/core/src/uint.rs +++ b/crates/core/src/uint.rs @@ -300,7 +300,7 @@ construct_uint! { BorshDeserializer, BorshSchema, )] - + #[repr(align(32))] pub struct Uint(4); } diff --git a/crates/ethereum_bridge/src/oracle/config.rs b/crates/ethereum_bridge/src/oracle/config.rs index 37e5a94cf98..bfd43787885 100644 --- a/crates/ethereum_bridge/src/oracle/config.rs +++ b/crates/ethereum_bridge/src/oracle/config.rs @@ -18,8 +18,7 @@ pub struct Config { pub active: bool, } -// TODO: this production Default implementation is temporary, there should be no -// default config - initialization should always be from storage. +#[cfg(any(test, feature = "testing"))] impl std::default::Default for Config { fn default() -> Self { Self { diff --git a/crates/ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs b/crates/ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs index f545110106a..b112b6c509f 100644 --- a/crates/ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs +++ b/crates/ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs @@ -18,9 +18,11 @@ use namada_core::ethereum_events::{ }; use namada_core::hints; use namada_core::storage::{BlockHeight, Key, KeySeg}; +use namada_core::uint::Uint; use namada_parameters::read_epoch_duration_parameter; use namada_state::{DBIter, StorageHasher, WlState, DB}; use namada_storage::{StorageRead, StorageWrite}; +use namada_trans_token::denominated; use namada_trans_token::storage_key::{balance_key, minted_balance_key}; use token::{burn_tokens, decrement_total_supply, increment_total_supply}; @@ -118,21 +120,27 @@ where let mut changed = if asset != &wrapped_native_erc20 { let (asset_count, changed) = mint_eth_assets(state, asset, receiver, amount)?; - // TODO: query denomination of the whitelisted token from storage, - // and print this amount with the proper formatting; for now, use - // NAM's formatting if asset_count.should_mint_erc20s() { + let denominated_amount = denominated( + asset_count.erc20_amount, + &erc20_token_address(asset), + state, + ) + .expect("The ERC20 token should have been whitelisted"); + tracing::info!( - "Minted wrapped ERC20s - (asset - {asset}, receiver - \ - {receiver}, amount - {})", - asset_count.erc20_amount.to_string_native(), + %asset, + %receiver, + %denominated_amount, + "Minted wrapped ERC20s", ); } if asset_count.should_mint_nuts() { tracing::info!( - "Minted NUTs - (asset - {asset}, receiver - {receiver}, \ - amount - {})", - asset_count.nut_amount.to_string_native(), + %asset, + %receiver, + undenominated_amount = %Uint::from(asset_count.nut_amount), + "Minted NUTs", ); } changed @@ -305,7 +313,10 @@ where return Ok((changed_keys, tx_events)); } - // TODO the timeout height is min_num_blocks of an epoch for now + // NB: the timeout height was chosen as the minimum number of + // blocks of an epoch. transfers that reside in the Bridge pool + // for a period longer than this number of blocks will be removed + // and refunded. let epoch_duration = read_epoch_duration_parameter(state)?; let timeout_offset = epoch_duration.min_num_of_blocks; diff --git a/crates/ethereum_bridge/src/protocol/transactions/utils.rs b/crates/ethereum_bridge/src/protocol/transactions/utils.rs index 995df8125d0..7a1b2c11a57 100644 --- a/crates/ethereum_bridge/src/protocol/transactions/utils.rs +++ b/crates/ethereum_bridge/src/protocol/transactions/utils.rs @@ -54,7 +54,6 @@ where Ok(voting_powers) } -// TODO: we might be able to remove allocation here pub(super) fn get_consensus_validators( state: &WlState, block_heights: HashSet, diff --git a/crates/ethereum_bridge/src/protocol/validation/ethereum_events.rs b/crates/ethereum_bridge/src/protocol/validation/ethereum_events.rs index 1ad96a28761..a51aea1efba 100644 --- a/crates/ethereum_bridge/src/protocol/validation/ethereum_events.rs +++ b/crates/ethereum_bridge/src/protocol/validation/ethereum_events.rs @@ -113,7 +113,8 @@ where // and if these are sorted in ascending order let have_dupes_or_non_sorted = { !ext.ethereum_events - // TODO: move to `array_windows` when it reaches Rust stable + // TODO(rust-lang/rust#75027): move to `array_windows` when it + // reaches Rust stable .windows(2) .all(|evs| evs[0] < evs[1]) }; diff --git a/crates/events/src/lib.rs b/crates/events/src/lib.rs index 3c8db7d82bc..78999b0a998 100644 --- a/crates/events/src/lib.rs +++ b/crates/events/src/lib.rs @@ -299,13 +299,6 @@ pub struct Event { attributes: BTreeMap, } -impl Display for Event { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // TODO: print attributes, too - write!(f, "{} in {}", self.event_type, self.level) - } -} - /// Errors to do with emitting events. #[derive(Error, Debug, Clone)] pub enum EventError { diff --git a/crates/ibc/src/event.rs b/crates/ibc/src/event.rs index 49cc7e8f57b..7bd613e8812 100644 --- a/crates/ibc/src/event.rs +++ b/crates/ibc/src/event.rs @@ -149,13 +149,16 @@ fn validate_ibc_event_type( let event_type = namada_event.kind().sub_domain(); + // TODO(namada#3229): validate IBC event types. eg: + // + // ```ignore // if !matches!( // event_type, - // // TODO: add other ibc event types that we use in namada // "update_client" | "send_packet" | "write_acknowledgement" - //) { - // return Err(EventError::InvalidEventType); - //} + // ) { + // return Err(EventError::InvalidEventType); + // } + // ``` Ok(IbcEventType(event_type.to_owned())) } diff --git a/crates/light_sdk/src/writing/asynchronous/mod.rs b/crates/light_sdk/src/writing/asynchronous/mod.rs index 3232b65e975..bb93b200893 100644 --- a/crates/light_sdk/src/writing/asynchronous/mod.rs +++ b/crates/light_sdk/src/writing/asynchronous/mod.rs @@ -26,9 +26,8 @@ pub async fn broadcast_tx( ) .map_err(|e| Error::Other(e.to_string()))?; - // TODO: configure an explicit timeout value? we need to hack away at - // `tendermint-rs` for this, which is currently using a hard-coded 30s - // timeout. + // NOTE: if we need an explicit client timeout, use + // `HttpClient::builder` to instantiate client let response = client .broadcast_tx_sync(tx.to_bytes()) .await diff --git a/crates/light_sdk/src/writing/blocking/mod.rs b/crates/light_sdk/src/writing/blocking/mod.rs index e82031fe2f5..ec0cd2a4cae 100644 --- a/crates/light_sdk/src/writing/blocking/mod.rs +++ b/crates/light_sdk/src/writing/blocking/mod.rs @@ -4,6 +4,7 @@ use namada_sdk::error::{EncodingError, Error, TxSubmitError}; use namada_sdk::queries::Client; use namada_sdk::tx::Tx; use tendermint_config::net::Address as TendermintAddress; +use tendermint_rpc::client::CompatMode; use tendermint_rpc::endpoint::broadcast::tx_sync::Response; use tendermint_rpc::error::Error as RpcError; use tendermint_rpc::HttpClient; @@ -18,11 +19,15 @@ use tokio::runtime::Runtime; /// /// In the case of errors in any of those stages, an error message is returned pub fn broadcast_tx(tendermint_addr: &str, tx: Tx) -> Result { - let client = HttpClient::new( + let client = HttpClient::builder( TendermintAddress::from_str(tendermint_addr) .map_err(|e| Error::Other(e.to_string()))?, ) + .compat_mode(CompatMode::V0_37) + .timeout(std::time::Duration::from_secs(30)) + .build() .map_err(|e| Error::Other(e.to_string()))?; + let rt = Runtime::new().unwrap(); let wrapper_tx_hash = tx.header_hash().to_string(); @@ -30,9 +35,6 @@ pub fn broadcast_tx(tendermint_addr: &str, tx: Tx) -> Result { // on-chain let decrypted_tx_hash = tx.raw_header_hash().to_string(); - // TODO: configure an explicit timeout value? we need to hack away at - // `tendermint-rs` for this, which is currently using a hard-coded 30s - // timeout. let response = rt .block_on(client.broadcast_tx_sync(tx.to_bytes())) .map_err(|e| Error::from(TxSubmitError::TxBroadcast(e)))?; diff --git a/crates/macros/src/lib.rs b/crates/macros/src/lib.rs index de74675e66e..66694edba8a 100644 --- a/crates/macros/src/lib.rs +++ b/crates/macros/src/lib.rs @@ -56,7 +56,7 @@ pub fn transaction(_attr: TokenStream, input: TokenStream) -> TokenStream { Ok(()) => 1, Err(err) => { namada_tx_prelude::debug_log!("Transaction error: {err}"); - // TODO: pass some proper error from txs, instead of a string + // TODO(namada#2980): pass some proper error from txs, instead of a string let err = err.to_string().serialize_to_vec(); ctx.yield_value(err); 0 @@ -165,7 +165,6 @@ pub fn derive_storage_keys(struct_def: TokenStream) -> TokenStream { } #[inline] -// TODO: use this crate for errors: https://crates.io/crates/proc-macro-error fn derive_storage_keys_inner(struct_def: TokenStream2) -> TokenStream2 { let struct_def: ItemStruct = syn::parse2(struct_def) .expect("Expected a struct in the StorageKeys derive"); diff --git a/crates/namada/src/ledger/governance/mod.rs b/crates/namada/src/ledger/governance/mod.rs index 5840f1a488d..65e56879d85 100644 --- a/crates/namada/src/ledger/governance/mod.rs +++ b/crates/namada/src/ledger/governance/mod.rs @@ -305,65 +305,9 @@ where .into()); } - // TODO: should any checks happen in the VP for the target validators? - // Not sure, since ultimately whether the vote counts or not is - // determined by the validator state at the end epoch, which likely has - // not occurred yet during VP execution - - // // Check the target validator of the bond used to vote - // let delegations_targets = find_delegation_validators( - // &self.ctx.pre(), - // voter, - // &pre_voting_end_epoch, - // ) - // .unwrap_or_default(); - - // if delegations_targets.is_empty() { - // return Err(native_vp::Error::new_alloc(format!( - // "No delegations found for {voter}" - // )) - // .into()); - // } - - // if !delegations_targets.contains(validator) { - // return Err(native_vp::Error::new_alloc(format!( - // "The vote key is not valid due to {voter} not having \ - // delegations to {validator}" - // )) - // .into()); - // } - - // let validator_state = read_validator_state(&self.ctx.pre(), - // validator, &pre_voting_end_epoch)?; if !matches! - // (validator_state, ValidatorState::Consensus | - // ValidatorState::BelowCapacity | ValidatorState::BelowThreshold) { - // if validator_state.is_none() { - // return Err(native_vp::Error::new_alloc(format!( - // "The vote key is invalid because the validator - // {validator} is not in \ the active set (jailed - // or inactive)" )) - // .into()); - // } else { - // return Err(native_vp::Error::new_alloc(format!( - // "The vote key is invalid because the validator - // {validator} is not in \ the active set (jailed - // or inactive)" )) - // .into()); - // } - - // } - - // // this is safe as we check above that validator is part of the - // hashmap if !matches!( - // delegations_targets.get(validator).unwrap(), - // ValidatorState::Consensus - // ) { - // return Err(native_vp::Error::new_alloc(format!( - // "The vote key is not valid due to {validator} being not in \ - // the active set" - // )) - // .into()); - // } + // No checks for the target validators, since ultimately whether the + // vote counts or not is determined by the validator state at the end + // epoch // Voted outside of voting window. We dont check for validator because // if the proposal type is validator, we need to let diff --git a/crates/namada/src/ledger/mod.rs b/crates/namada/src/ledger/mod.rs index f4465826ed6..14c4a8b6220 100644 --- a/crates/namada/src/ledger/mod.rs +++ b/crates/namada/src/ledger/mod.rs @@ -234,9 +234,10 @@ mod test { tx_wasm_cache: self.tx_wasm_cache.clone(), storage_read_past_height_limit: None, }; - // TODO: this is a hack to propagate errors to the caller, we should - // really permit error types other than [`std::io::Error`] - if request.path == "/shell/dry_run_tx" { + // TODO(namada#3240): this is a hack to propagate errors to the + // caller, we should really permit error types other + // than [`std::io::Error`] + if request.path == RPC.shell().dry_run_tx_path() { super::dry_run_tx(ctx, &request) } else { self.rpc.handle(ctx, &request) diff --git a/crates/namada/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs b/crates/namada/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs index 23be647a845..858adf3e7b3 100644 --- a/crates/namada/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs +++ b/crates/namada/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs @@ -598,8 +598,9 @@ where self.determine_escrow_checks(&wnam_address, &transfer)?; if !escrow_checks.validate(keys_changed) { let error = native_vp::Error::new_const( - // TODO: specify which storage changes are missing - "Missing storage modifications in the Bridge pool", + // TODO(namada#3247): specify which storage changes are missing + // or which ones are invalid + "Invalid storage modifications in the Bridge pool", ) .into(); tracing::debug!("{error}"); diff --git a/crates/namada/src/ledger/pgf/mod.rs b/crates/namada/src/ledger/pgf/mod.rs index 4c29c13b3b7..ff042286fea 100644 --- a/crates/namada/src/ledger/pgf/mod.rs +++ b/crates/namada/src/ledger/pgf/mod.rs @@ -113,8 +113,8 @@ where match key_type { KeyType::Stewards(steward_address) => { let stewards_have_increased = { - // TODO: maybe we should check errors here, which could - // be out-of-gas related? + // TODO(namada#3238): we should check errors here, which + // could be out-of-gas related let total_stewards_pre = pgf_storage::stewards_handle() .len(&self.ctx.pre()) .unwrap_or_default(); diff --git a/crates/namada/src/lib.rs b/crates/namada/src/lib.rs index cc283c520c0..c7274fff70b 100644 --- a/crates/namada/src/lib.rs +++ b/crates/namada/src/lib.rs @@ -17,6 +17,7 @@ clippy::print_stderr )] +// TODO(namada#3248): only re-export v037 `tendermint-rs` pub use namada_core::{ address, chain, dec, decode, encode, eth_abi, eth_bridge_pool, ethereum_events, ethereum_structs, hash, internal, keccak, key, masp, @@ -24,6 +25,7 @@ pub use namada_core::{ validity_predicate, voting_power, }; pub use namada_sdk::{control_flow, io}; +// TODO(namada#3248): only re-export v037 `tendermint-rs` #[cfg(feature = "tendermint-rpc")] pub use tendermint_rpc; pub use { diff --git a/crates/namada/src/vm/wasm/run.rs b/crates/namada/src/vm/wasm/run.rs index 30c58c1cd2d..002081a2885 100644 --- a/crates/namada/src/vm/wasm/run.rs +++ b/crates/namada/src/vm/wasm/run.rs @@ -41,7 +41,7 @@ const VP_ENTRYPOINT: &str = "_validate_tx"; const WASM_STACK_LIMIT: u32 = u16::MAX as u32; /// The error type returned by transactions. -// TODO: move this to `core`, to be shared with the wasm vm, +// TODO(namada#2980): move this to `core`, to be shared with the wasm vm, // and make it an `enum` of different variants type TxError = String; diff --git a/crates/node/src/bench_utils.rs b/crates/node/src/bench_utils.rs index 19dccb4f70f..da0828ecc7e 100644 --- a/crates/node/src/bench_utils.rs +++ b/crates/node/src/bench_utils.rs @@ -796,7 +796,7 @@ impl Client for BenchShell { storage_read_past_height_limit: None, }; - if request.path == "/shell/dry_run_tx" { + if request.path == RPC.shell().dry_run_tx_path() { dry_run_tx(ctx, &request) } else { RPC.handle(ctx, &request) diff --git a/crates/node/src/broadcaster.rs b/crates/node/src/broadcaster.rs index ec616003d2d..8a081f19373 100644 --- a/crates/node/src/broadcaster.rs +++ b/crates/node/src/broadcaster.rs @@ -5,6 +5,7 @@ use namada::control_flow::time; use namada::time::{DateTimeUtc, Utc}; use tokio::sync::mpsc::UnboundedReceiver; +use crate::facade::tendermint_rpc::client::CompatMode; use crate::facade::tendermint_rpc::{Client, HttpClient}; const DEFAULT_BROADCAST_TIMEOUT: u64 = 180; @@ -23,8 +24,13 @@ impl Broadcaster { /// over the given url. pub fn new(url: SocketAddr, receiver: UnboundedReceiver>) -> Self { Self { - client: HttpClient::new(format!("http://{}", url).as_str()) - .unwrap(), + client: HttpClient::builder( + format!("http://{}", url).as_str().try_into().unwrap(), + ) + .compat_mode(CompatMode::V0_37) + .timeout(std::time::Duration::from_secs(30)) + .build() + .unwrap(), receiver, } } diff --git a/crates/node/src/ethereum_oracle/mod.rs b/crates/node/src/ethereum_oracle/mod.rs index c0b49a4db4a..ed454eb0023 100644 --- a/crates/node/src/ethereum_oracle/mod.rs +++ b/crates/node/src/ethereum_oracle/mod.rs @@ -116,7 +116,6 @@ impl RpcClient for Provider { where Self: Sized, { - // TODO: return a result here? Provider::::try_from(url).expect("Invalid Ethereum RPC url") } @@ -995,8 +994,6 @@ mod test_oracle { } // check that the oracle hasn't yet checked any further blocks - // TODO: check this in a deterministic way rather than just waiting a - // bit assert!( timeout( std::time::Duration::from_secs(1), @@ -1093,8 +1090,6 @@ mod test_oracle { assert_eq!(block_processed, Uint256::from(confirmed_block_height - 4)); // check that the oracle hasn't yet checked any further blocks - // TODO: check this in a deterministic way rather than just waiting a - // bit let res = timeout( std::time::Duration::from_secs(3), blocks_processed_recv.recv(), diff --git a/crates/node/src/shell/block_alloc.rs b/crates/node/src/shell/block_alloc.rs index 321e5d8cd6d..ef723fe1f48 100644 --- a/crates/node/src/shell/block_alloc.rs +++ b/crates/node/src/shell/block_alloc.rs @@ -32,16 +32,12 @@ pub mod states; -// TODO: what if a tx has a size greater than the threshold for -// its bin? how do we handle this? if we keep it in the mempool +// TODO(namada#3250): what if a tx has a size greater than the threshold +// for its bin? how do we handle this? if we keep it in the mempool // forever, it'll be a DoS vec, as we can make nodes run out of // memory! maybe we should allow block decisions for txs that are // too big to fit in their respective bin? in these special block // decisions, we would only decide proposals with "large" txs?? -// -// MAYBE: in the state machine impl, reset to beginning state, and -// and alloc space for large tx right at the start. the problem with -// this is that then we may not have enough space for decrypted txs use std::marker::PhantomData; diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 1a0b458f07d..257d16216e3 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -4582,7 +4582,7 @@ mod test_finalize_block { ); // Check the balance of the Slash Pool - // TODO: finish once implemented + // TODO(namada#2984): finish once implemented // let slash_pool_balance: token::Amount = shell // .state // .read(&slash_balance_key) @@ -4605,7 +4605,7 @@ mod test_finalize_block { assert_eq!(current_epoch.0, 10_u64); // Check the balance of the Slash Pool - // TODO: finish once implemented + // TODO(namada#2984): finish once implemented // let slash_pool_balance: token::Amount = shell // .state // .read(&slash_balance_key) @@ -4663,7 +4663,7 @@ mod test_finalize_block { // dbg!(pre_stake_10 - post_stake_10); // dbg!(&exp_slashed_during_processing_9); - // TODO: finish once implemented + // TODO(namada#2984): finish once implemented // assert!( // ((pre_stake_11 - post_stake_11).change() - // exp_slashed_4.change()) .abs() @@ -4834,7 +4834,7 @@ mod test_finalize_block { <= Uint::one() ); - // TODO: finish once implemented + // TODO(namada#2984): finish once implemented // Check the balance of the Slash Pool // let slash_pool_balance: token::Amount = shell // .state diff --git a/crates/node/src/shell/init_chain.rs b/crates/node/src/shell/init_chain.rs index 3f81d9b3fed..457004ff2a4 100644 --- a/crates/node/src/shell/init_chain.rs +++ b/crates/node/src/shell/init_chain.rs @@ -220,7 +220,7 @@ where ) -> ControlFlow<()> { let ts: protobuf::Timestamp = init.time.into(); let initial_height = init.initial_height.into(); - // TODO hacky conversion, depends on https://github.com/informalsystems/tendermint-rs/issues/870 + // TODO(informalsystems/tendermint-rs#870): hacky conversion let genesis_time: DateTimeUtc = (Utc.timestamp_opt( ts.seconds, u32::try_from(ts.nanos).expect("Time nanos cannot be negative"), @@ -628,8 +628,6 @@ where .write(&protocol_pk_key(address), &protocol_key.pk.raw) .expect("Unable to set genesis user protocol public key"); - // TODO: replace pos::init_genesis validators arg with - // init_genesis_validator from here if let Err(err) = pos::namada_proof_of_stake::become_validator( &mut self.state, BecomeValidator { diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index 610f43d0c6c..cecdb51ecab 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -529,7 +529,7 @@ where ), ), storage_read_past_height_limit, - // TODO: config event log params + // TODO(namada#3237): config event log params event_log: EventLog::default(), }; shell.update_eth_oracle(&Default::default()); @@ -680,19 +680,13 @@ where .borrow() .as_ref() .cloned(); - match last_processed_block { - Some(eth_height) => { - tracing::info!( - "Ethereum oracle's most recently processed Ethereum \ - block is {}", - eth_height - ); - self.state.in_mem_mut().ethereum_height = Some(eth_height); - } - None => tracing::info!( - "Ethereum oracle has not yet fully processed any Ethereum \ - blocks" - ), + if let Some(eth_height) = last_processed_block { + tracing::info!( + "Ethereum oracle's most recently processed Ethereum block \ + is {}", + eth_height + ); + self.state.in_mem_mut().ethereum_height = Some(eth_height); } } } @@ -797,14 +791,14 @@ where in storage or not", ); if !has_key { - tracing::info!( + tracing::debug!( "Not starting oracle yet as storage has not been \ initialized" ); return; } let Some(config) = EthereumOracleConfig::read(&self.state) else { - tracing::info!( + tracing::debug!( "Not starting oracle as the Ethereum bridge config \ couldn't be found in storage" ); @@ -814,13 +808,13 @@ where if !changed_keys .contains(&namada::eth_bridge::storage::active_key()) { - tracing::info!( + tracing::debug!( "Not starting oracle as the Ethereum bridge is \ disabled" ); return; } else { - tracing::info!( + tracing::debug!( "Disabling oracle as the bridge has been disabled" ); false @@ -838,7 +832,7 @@ where tracing::info!( ?start_block, "Found Ethereum height from which the Ethereum oracle should \ - start" + be updated" ); let config = namada::eth_bridge::oracle::config::Config { min_confirmations: config.min_confirmations.into(), @@ -848,7 +842,7 @@ where }; tracing::info!( ?config, - "Starting the Ethereum oracle using values from block storage" + "Updating the Ethereum oracle using values from block storage" ); if let Err(error) = control_sender .try_send(oracle::control::Command::UpdateConfig(config)) diff --git a/crates/node/src/shell/prepare_proposal.rs b/crates/node/src/shell/prepare_proposal.rs index 6b5f5e9a3e3..6cc2f22392e 100644 --- a/crates/node/src/shell/prepare_proposal.rs +++ b/crates/node/src/shell/prepare_proposal.rs @@ -150,7 +150,7 @@ where false } AllocFailure::OverflowsBin { bin_resource} => { - // TODO: handle tx whose size is greater + // TODO(namada#3250): handle tx whose size is greater // than bin size tracing::warn!( ?tx_bytes, @@ -216,7 +216,7 @@ where .map_or_else( |status| match status { AllocFailure::Rejected { bin_resource_left} => { - // TODO: maybe we should find a way to include + // TODO(namada#3250): maybe we should find a way to include // validator set updates all the time. for instance, // we could have recursive bins -> bin space within // a bin is partitioned into yet more bins. so, we @@ -234,7 +234,7 @@ where false } AllocFailure::OverflowsBin { bin_resource} => { - // TODO: handle tx whose size is greater + // TODO(namada#3250): handle tx whose size is greater // than bin size tracing::warn!( ?tx_bytes, @@ -401,7 +401,7 @@ where #[allow(clippy::cast_possible_wrap, clippy::cast_possible_truncation)] #[cfg(test)] -// TODO: write tests for validator set update vote extensions in +// TODO(namada#3249): write tests for validator set update vote extensions in // prepare proposals mod test_prepare_proposal { use std::collections::BTreeSet; diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index bc211c6aece..f226097c116 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -550,6 +550,8 @@ where /// We test the failure cases of [`process_proposal`]. The happy flows /// are covered by the e2e tests. +// TODO(namada#3249): write tests for validator set update vote extensions in +// process proposals #[cfg(test)] mod test_process_proposal { use namada::core::key::*; diff --git a/crates/node/src/shell/queries.rs b/crates/node/src/shell/queries.rs index ad9d9bebc26..a1490b4a082 100644 --- a/crates/node/src/shell/queries.rs +++ b/crates/node/src/shell/queries.rs @@ -1,7 +1,7 @@ //! Shell methods for querying state use namada::ledger::dry_run_tx; -use namada::ledger::queries::{RequestCtx, ResponseQuery}; +use namada::ledger::queries::{RequestCtx, ResponseQuery, RPC}; use super::*; @@ -24,7 +24,7 @@ where }; // Invoke the root RPC handler - returns borsh-encoded data on success - let result = if query.path == "/shell/dry_run_tx" { + let result = if query.path == RPC.shell().dry_run_tx_path() { dry_run_tx(ctx, &query) } else { namada::ledger::queries::handle_path(ctx, &query) diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index 22ad689d5d1..279c7ba4331 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -717,7 +717,7 @@ impl<'a> Client for &'a MockNode { tx_wasm_cache: borrowed.tx_wasm_cache.read_only(), storage_read_past_height_limit: None, }; - if request.path == "/shell/dry_run_tx" { + if request.path == RPC.shell().dry_run_tx_path() { dry_run_tx(ctx, &request) } else { rpc.handle(ctx, &request) diff --git a/crates/node/src/shell/vote_extensions/val_set_update.rs b/crates/node/src/shell/vote_extensions/val_set_update.rs index 389ee7eba84..ed87a89896f 100644 --- a/crates/node/src/shell/vote_extensions/val_set_update.rs +++ b/crates/node/src/shell/vote_extensions/val_set_update.rs @@ -403,13 +403,4 @@ mod test_vote_extensions { .is_err() ); } - - /// Test if a [`validator_set_update::Vext`] is signed with a secp key - /// that belongs to a consensus validator of some previous epoch - #[test] - #[ignore] - fn test_secp_key_belongs_to_consensus_validator() { - // TODO: we need to prove ownership of validator keys - // https://github.com/anoma/namada/issues/106 - } } diff --git a/crates/proof_of_stake/src/lib.rs b/crates/proof_of_stake/src/lib.rs index 92326128bc9..749c1121d41 100644 --- a/crates/proof_of_stake/src/lib.rs +++ b/crates/proof_of_stake/src/lib.rs @@ -176,8 +176,9 @@ pub fn is_validator( where S: StorageRead, { - // TODO: should this check be made different? I suppose it does work but - // feels weird... + // NB: we attempt to read one of the validator keys in storage. + // kinda weird, but it works to check if `address` belongs to + // a validator let rate = read_validator_max_commission_rate_change(storage, address)?; Ok(rate.is_some()) } diff --git a/crates/proof_of_stake/src/tests/state_machine.rs b/crates/proof_of_stake/src/tests/state_machine.rs index 459da961cb8..82beb9ca155 100644 --- a/crates/proof_of_stake/src/tests/state_machine.rs +++ b/crates/proof_of_stake/src/tests/state_machine.rs @@ -1004,9 +1004,6 @@ impl ConcretePosState { // Post-condition: the validator stake at the pipeline should be // decremented at most by the bond amount (because slashing can reduce // the actual amount unbonded) - // - // TODO: is this a weak assertion here? Seems cumbersome to calculate - // the exact amount considering the slashing applied can be complicated assert!( stake_at_pipeline >= validator_stake_before_unbond_pipeline @@ -2175,7 +2172,6 @@ impl ConcretePosState { } } } - // TODO: expand this to include jailed validators } } @@ -4776,7 +4772,6 @@ impl AbstractPosState { .unwrap_or_default() .iter() .filter(|&slash| { - // TODO: check bounds! start <= slash.epoch && slash.epoch + self.params.slash_processing_epoch_offset() <= epoch @@ -5738,7 +5733,6 @@ impl AbstractPosState { let updated_amount = computed_slashes .iter() .filter(|(&epoch, _)| { - // TODO: check if bounds correct! // slashes that have already been applied and processed epoch + params.slash_processing_epoch_offset() <= slash.epoch }) diff --git a/crates/proof_of_stake/src/tests/utils.rs b/crates/proof_of_stake/src/tests/utils.rs index 1e7deac4f4c..0b58937c60c 100644 --- a/crates/proof_of_stake/src/tests/utils.rs +++ b/crates/proof_of_stake/src/tests/utils.rs @@ -6,7 +6,6 @@ use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering::Relaxed; use std::{env, fmt}; -// TODO: allow custom fmt fn #[derive(Clone)] pub struct DbgPrintDiff where diff --git a/crates/sdk/src/events/log.rs b/crates/sdk/src/events/log.rs index 5409006dab4..8ca19e17e76 100644 --- a/crates/sdk/src/events/log.rs +++ b/crates/sdk/src/events/log.rs @@ -23,7 +23,7 @@ pub struct Params { impl Default for Params { fn default() -> Self { - // TODO: tune the default params + // TODO(namada#3237): tune the default params Self { max_log_events_per_kind: 50000, } diff --git a/crates/sdk/src/queries/mod.rs b/crates/sdk/src/queries/mod.rs index ee8869c0194..eb6232cf71b 100644 --- a/crates/sdk/src/queries/mod.rs +++ b/crates/sdk/src/queries/mod.rs @@ -180,8 +180,6 @@ mod testing { tx_wasm_cache: (), storage_read_past_height_limit: None, }; - // TODO: this is a hack to propagate errors to the caller, we should - // really permit error types other than [`std::io::Error`] self.rpc.handle(ctx, &request).map_err(|err| { std::io::Error::new(std::io::ErrorKind::Other, err.to_string()) }) @@ -353,8 +351,6 @@ pub trait Client { where H: Into + Send, { - // TODO(tarcieri): return errors for invalid params before making - // request? self.perform(blockchain::Request::new(min.into(), max.into())) .await } @@ -437,6 +433,8 @@ impl Client for C { height: Option, prove: bool, ) -> Result { + use crate::tendermint::abci::Code; + let data = data.unwrap_or_default(); let height = height .map(|height| { @@ -444,16 +442,8 @@ impl Client for C { .map_err(|_err| Error::InvalidHeight(height)) }) .transpose()?; - let response = self - .abci_query( - // TODO open the private Path constructor in tendermint-rpc - Some(std::str::FromStr::from_str(&path).unwrap()), - data, - height, - prove, - ) - .await?; - use crate::tendermint::abci::Code; + + let response = self.abci_query(Some(path), data, height, prove).await?; match response.code { Code::Ok => Ok(EncodedResponseQuery { data: response.value, diff --git a/crates/sdk/src/queries/router.rs b/crates/sdk/src/queries/router.rs index e9adfe622e2..ec2e5fe3d80 100644 --- a/crates/sdk/src/queries/router.rs +++ b/crates/sdk/src/queries/router.rs @@ -363,8 +363,6 @@ macro_rules! try_match { } /// Convert literal pattern into a `&[&'static str]` -// TODO sub router pattern is not yet used -#[allow(unused_macros)] macro_rules! pattern_to_prefix { ( ( $( $pattern:literal )/ * ) ) => { &[$( $pattern ),*] @@ -809,7 +807,7 @@ macro_rules! router { router_type!{[<$name:camel>] {}, $( $pattern $( -> $return_type )? = $handle ),* } impl $crate::queries::Router for [<$name:camel>] { - // TODO: for some patterns, there's unused assignment of `$end` + // NB: for some patterns, there's an unused assignment of `$end` #[allow(unused_assignments)] fn internal_handle( &self, diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index 30921743b92..f6c5e4f8407 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -533,7 +533,6 @@ pub async fn validate_transparent_fee( /// Create a wrapper tx from a normal tx. Get the hash of the /// wrapper and its payload which is needed for monitoring its /// progress on chain. -#[allow(clippy::too_many_arguments)] pub async fn wrap_tx( tx: &mut Tx, args: &args::Tx, @@ -546,7 +545,7 @@ pub async fn wrap_tx( token: args.fee_token.clone(), }, fee_payer, - // TODO: partially validate the gas limit in client + // TODO(namada#1625): partially validate the gas limit in client args.gas_limit, ); diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index d569c24887b..5dc90cec700 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -198,7 +198,6 @@ pub fn dump_tx(io: &IO, args: &args::Tx, tx: Tx) { /// Prepare a transaction for signing and submission by adding a wrapper header /// to it. -#[allow(clippy::too_many_arguments)] pub async fn prepare_tx( args: &args::Tx, tx: &mut Tx, @@ -238,9 +237,6 @@ pub async fn process_tx( // We use this to determine when the decrypted inner tx makes it // on-chain let to_broadcast = TxBroadcastData::Live { tx, tx_hash }; - // TODO: implement the code to resubmit the wrapper if it fails because - // of masp epoch Either broadcast or submit transaction and - // collect result into sum type if args.broadcast_only { broadcast_tx(context, &to_broadcast) .await @@ -331,9 +327,6 @@ pub async fn broadcast_tx( "Broadcasting transaction", ); - // TODO: configure an explicit timeout value? we need to hack away at - // `tendermint-rs` for this, which is currently using a hard-coded 30s - // timeout. let response = lift_rpc_error( context.client().broadcast_tx_sync(tx.to_bytes()).await, )?; @@ -1999,7 +1992,6 @@ pub async fn build_default_proposal( }; Ok(()) }; - // TODO: need to pay the fee to submit a proposal, check enough balance build( context, tx, @@ -2374,7 +2366,6 @@ pub async fn build_pgf_funding_proposal( let (fee_amount, _updated_balance) = validate_transparent_fee(context, tx, &signing_data.fee_payer).await?; - // TODO: need to pay the fee to submit a proposal, check enough balance let init_proposal_data = InitProposalData::try_from(proposal.clone()) .map_err(|e| TxSubmitError::InvalidProposal(e.to_string()))?; @@ -2420,7 +2411,6 @@ pub async fn build_pgf_stewards_proposal( let (fee_amount, _updated_balance) = validate_transparent_fee(context, tx, &signing_data.fee_payer).await?; - // TODO: need to pay the fee to submit a proposal, check enough balance let init_proposal_data = InitProposalData::try_from(proposal.clone()) .map_err(|e| TxSubmitError::InvalidProposal(e.to_string()))?; @@ -2901,7 +2891,8 @@ pub async fn build_transfer( // This has no side-effect because transaction is to self. let (transparent_amount, transparent_token) = if source == masp_addr && target == masp_addr { - // TODO Refactor me, we shouldn't rely on any specific token here. + // TODO(namada#1677): Refactor me, we shouldn't rely on any specific + // token here. (token::Amount::zero().into(), context.native_token()) } else { (validated_amount, args.token.clone()) diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index 790e51c866d..01db64feb86 100644 --- a/crates/sdk/src/wallet/mod.rs +++ b/crates/sdk/src/wallet/mod.rs @@ -772,9 +772,11 @@ impl Wallet { alias = format!("disposable_{ctr}"); } // Generate a disposable keypair to sign the wrapper if requested - // TODO: once the wrapper transaction has been applied, this key can be - // deleted from wallet (the transaction being accepted is not enough - // cause we could end up doing a rollback) + // + // TODO(namada#3239): once the wrapper transaction has been applied, + // this key can be deleted from wallet (the transaction being + // accepted is not enough cause we could end up doing a + // rollback) let (alias, disposable_keypair) = self .gen_store_secret_key( SchemeType::Ed25519, diff --git a/crates/shielded_token/src/lib.rs b/crates/shielded_token/src/lib.rs index b14965fc0ed..b586ea2ddd5 100644 --- a/crates/shielded_token/src/lib.rs +++ b/crates/shielded_token/src/lib.rs @@ -24,6 +24,7 @@ pub mod utils; use std::str::FromStr; +pub use masp_primitives::transaction; use namada_core::borsh::{BorshDeserialize, BorshSchema, BorshSerialize}; use namada_core::dec::Dec; pub use namada_storage::conversion_state::{ @@ -55,7 +56,7 @@ pub struct ShieldedParams { /// Shielded Pool nominal proportional gain for the given token pub kp_gain_nom: Dec, /// Target amount for the given token that is locked in the shielded pool - /// TODO: should this be a Uint or DenominatedAmount??? + // TODO(namada#3255): use `Uint` here pub locked_amount_target: u64, } diff --git a/crates/tx/src/data/wrapper.rs b/crates/tx/src/data/wrapper.rs index dc97b3968ed..23bd4c53f15 100644 --- a/crates/tx/src/data/wrapper.rs +++ b/crates/tx/src/data/wrapper.rs @@ -39,7 +39,7 @@ pub enum WrapperTxErr { DenominatedFeeConversion, } -/// A fee is an amount of a specified token +/// Amount of some specified token to pay for fees. #[derive( Debug, Clone, @@ -53,10 +53,9 @@ pub enum WrapperTxErr { Eq, )] pub struct Fee { - /// amount of fee per gas unit + /// Amount of fees paid per gas unit. pub amount_per_gas_unit: DenominatedAmount, - /// address of the token - /// TODO: This should support multi-tokens + /// Address of the fee token. pub token: Address, } diff --git a/crates/vote_ext/src/bridge_pool_roots.rs b/crates/vote_ext/src/bridge_pool_roots.rs index 5a36d5fb3ea..586cfcaf124 100644 --- a/crates/vote_ext/src/bridge_pool_roots.rs +++ b/crates/vote_ext/src/bridge_pool_roots.rs @@ -32,10 +32,11 @@ use namada_tx::Signed; BorshSchema, )] pub struct BridgePoolRootVext { - /// The validator sending the vote extension - /// TODO: the validator's address is temporarily being included - /// until we're able to map a Tendermint address to a validator - /// address (see ) + /// The address of the validator who submitted the vote extension. + // NOTE: The validator's established address was included as a workaround + // for `namada#200`, which prevented us from mapping a CometBFT validator + // address to a Namada address. Since then, we have committed to keeping + // this `validator_addr` field. pub validator_addr: Address, /// The block height at which the vote extensions was /// sent. diff --git a/crates/vote_ext/src/ethereum_events.rs b/crates/vote_ext/src/ethereum_events.rs index 683705f275f..0dc94c4a28d 100644 --- a/crates/vote_ext/src/ethereum_events.rs +++ b/crates/vote_ext/src/ethereum_events.rs @@ -62,9 +62,11 @@ impl From> for SignedVext { pub struct EthereumEventsVext { /// The block height for which this [`Vext`] was made. pub block_height: BlockHeight, - /// TODO: the validator's address is temporarily being included - /// until we're able to map a Tendermint address to a validator - /// address (see ) + /// The address of the validator who submitted the vote extension. + // NOTE: The validator's established address was included as a workaround + // for `namada#200`, which prevented us from mapping a CometBFT validator + // address to a Namada address. Since then, we have committed to keeping + // this `validator_addr` field. pub validator_addr: Address, /// The new ethereum events seen. These should be /// deterministically ordered. @@ -170,10 +172,6 @@ impl VextDigest { } } - // TODO: we probably need a manual `Ord` impl for - // `EthereumEvent`, such that this `sort()` is - // always deterministic, regardless - // of crate versions changing and such ext.ethereum_events.sort(); let signed = Signed::new_from(ext, sig); diff --git a/crates/vote_ext/src/validator_set_update.rs b/crates/vote_ext/src/validator_set_update.rs index 5bc9367bd75..a5ae7116f5c 100644 --- a/crates/vote_ext/src/validator_set_update.rs +++ b/crates/vote_ext/src/validator_set_update.rs @@ -19,7 +19,7 @@ use namada_migrations::*; use namada_tx::Signed; // the contract versions and namespaces plugged into validator set hashes -// TODO: ideally, these values should not be hardcoded +// TODO(namada#249): ideally, these values should not be hardcoded const BRIDGE_CONTRACT_VERSION: u8 = 1; const BRIDGE_CONTRACT_NAMESPACE: &str = "bridge"; const GOVERNANCE_CONTRACT_VERSION: u8 = 1; @@ -128,9 +128,11 @@ pub struct ValidatorSetUpdateVext { /// values. The arrays are sorted in descending order based /// on the voting power of each validator. pub voting_powers: VotingPowersMap, - /// TODO: the validator's address is temporarily being included - /// until we're able to map a Tendermint address to a validator - /// address (see ) + /// The address of the validator who submitted the vote extension. + // NOTE: The validator's established address was included as a workaround + // for `namada#200`, which prevented us from mapping a CometBFT validator + // address to a Namada address. Since then, we have committed to keeping + // this `validator_addr` field. pub validator_addr: Address, /// The value of Namada's [`Epoch`] at the creation of this /// [`Vext`]. @@ -363,7 +365,6 @@ fn encode_validator_data( BorshDeserializer, BorshSchema, )] -// TODO: find a new home for this type pub struct ValidatorSetArgs { /// Ethereum addresses of the validators. pub validators: Vec, diff --git a/crates/vp_env/src/lib.rs b/crates/vp_env/src/lib.rs index b3115443dc9..a4931c31b7d 100644 --- a/crates/vp_env/src/lib.rs +++ b/crates/vp_env/src/lib.rs @@ -20,7 +20,6 @@ pub mod collection_validation; -// TODO: this should be re-exported from namada_shielded_token use masp_primitives::transaction::Transaction; use namada_core::address::Address; use namada_core::borsh::BorshDeserialize; diff --git a/wasm/tx_change_consensus_key/src/lib.rs b/wasm/tx_change_consensus_key/src/lib.rs index 603de8cfdc9..ad17c70b605 100644 --- a/wasm/tx_change_consensus_key/src/lib.rs +++ b/wasm/tx_change_consensus_key/src/lib.rs @@ -4,7 +4,7 @@ use booleans::ResultBoolExt; use namada_tx_prelude::transaction::pos::ConsensusKeyChange; use namada_tx_prelude::*; -#[transaction] // TODO: need to benchmark this gas +#[transaction] fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { let data = ctx.get_tx_data(&tx_data)?; let ConsensusKeyChange { diff --git a/wasm/tx_change_validator_metadata/src/lib.rs b/wasm/tx_change_validator_metadata/src/lib.rs index 8be4cc62ed1..ad8ae30fb8a 100644 --- a/wasm/tx_change_validator_metadata/src/lib.rs +++ b/wasm/tx_change_validator_metadata/src/lib.rs @@ -4,7 +4,6 @@ use namada_tx_prelude::transaction::pos::MetaDataChange; use namada_tx_prelude::*; -// TODO: need to benchmark gas!!! #[transaction] fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { let data = ctx.get_tx_data(&tx_data)?; diff --git a/wasm/tx_claim_rewards/src/lib.rs b/wasm/tx_claim_rewards/src/lib.rs index b93f0595063..eedd62c2313 100644 --- a/wasm/tx_claim_rewards/src/lib.rs +++ b/wasm/tx_claim_rewards/src/lib.rs @@ -3,7 +3,7 @@ use namada_tx_prelude::*; -#[transaction] // TODO: needs to be benchmarked +#[transaction] fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { let data = ctx.get_tx_data(&tx_data)?; let withdraw = transaction::pos::Withdraw::try_from_slice(&data[..]) diff --git a/wasm/tx_redelegate/src/lib.rs b/wasm/tx_redelegate/src/lib.rs index fc5f838f022..efc7aab65ed 100644 --- a/wasm/tx_redelegate/src/lib.rs +++ b/wasm/tx_redelegate/src/lib.rs @@ -62,7 +62,6 @@ mod tests { } } - // TODO: more assertions needed!! fn test_tx_redelegate_aux( initial_stake: token::Amount, redelegation: transaction::pos::Redelegation,