Skip to content

Commit

Permalink
Audit TODOs in codebase
Browse files Browse the repository at this point in the history
  • Loading branch information
sug0 committed May 17, 2024
1 parent 4ed6229 commit 7dba310
Show file tree
Hide file tree
Showing 70 changed files with 222 additions and 320 deletions.
2 changes: 1 addition & 1 deletion crates/apps/src/lib/bench_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,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)
Expand Down
1 change: 0 additions & 1 deletion crates/apps/src/lib/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3381,7 +3381,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,
Expand Down
7 changes: 6 additions & 1 deletion crates/apps/src/lib/cli/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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(
Expand Down
1 change: 0 additions & 1 deletion crates/apps/src/lib/cli/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion crates/apps/src/lib/cli/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
11 changes: 9 additions & 2 deletions crates/apps/src/lib/client/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1058,13 +1058,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!(
Expand Down
1 change: 0 additions & 1 deletion crates/apps/src/lib/config/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,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()
Expand Down
4 changes: 2 additions & 2 deletions crates/apps/src/lib/config/genesis/templates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down
2 changes: 1 addition & 1 deletion crates/apps/src/lib/config/genesis/transactions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,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)
Expand Down
5 changes: 2 additions & 3 deletions crates/apps/src/lib/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion crates/apps/src/lib/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,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;
pub mod tower_abci {
Expand Down
10 changes: 8 additions & 2 deletions crates/apps/src/lib/node/ledger/broadcaster.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::ops::ControlFlow;

use namada::control_flow::time;
use namada::time::{DateTimeUtc, Utc};
use tendermint_rpc::client::CompatMode;
use tokio::sync::mpsc::UnboundedReceiver;

use crate::facade::tendermint_rpc::{Client, HttpClient};
Expand All @@ -23,8 +24,13 @@ impl Broadcaster {
/// over the given url.
pub fn new(url: SocketAddr, receiver: UnboundedReceiver<Vec<u8>>) -> 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,
}
}
Expand Down
5 changes: 0 additions & 5 deletions crates/apps/src/lib/node/ledger/ethereum_oracle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ impl RpcClient for Provider<Http> {
where
Self: Sized,
{
// TODO: return a result here?
Provider::<Http>::try_from(url).expect("Invalid Ethereum RPC url")
}

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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(),
Expand Down
8 changes: 2 additions & 6 deletions crates/apps/src/lib/node/ledger/shell/block_alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
8 changes: 4 additions & 4 deletions crates/apps/src/lib/node/ledger/shell/finalize_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4204,7 +4204,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)
Expand All @@ -4227,7 +4227,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)
Expand Down Expand Up @@ -4285,7 +4285,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()
Expand Down Expand Up @@ -4456,7 +4456,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
Expand Down
4 changes: 1 addition & 3 deletions crates/apps/src/lib/node/ledger/shell/init_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,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, ts.nanos as u32))
.single()
Expand Down Expand Up @@ -627,8 +627,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 {
Expand Down
34 changes: 14 additions & 20 deletions crates/apps/src/lib/node/ledger/shell/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ where
tx_wasm_compilation_cache as usize,
),
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());
Expand Down Expand Up @@ -673,19 +673,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);
}
}
}
Expand Down Expand Up @@ -790,14 +784,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"
);
Expand All @@ -807,13 +801,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
Expand All @@ -831,7 +825,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(),
Expand All @@ -841,7 +835,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))
Expand Down
8 changes: 4 additions & 4 deletions crates/apps/src/lib/node/ledger/shell/prepare_proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -371,7 +371,7 @@ where
}

#[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;
Expand Down
2 changes: 2 additions & 0 deletions crates/apps/src/lib/node/ledger/shell/process_proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,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::*;
Expand Down
Loading

0 comments on commit 7dba310

Please sign in to comment.