ronantakizawa/codereview-bench huggingface dataset:
Repository: paritytech/substrate
File: client/network/src/service/out_events.rs (Rust)

Code before:
			let current_pending = sender.inner.len();
			if current_pending >= sender.queue_size_warning && !sender.warning_fired {
				sender.warning_fired = true;
				error!(
					"The number of unprocessed events in channel `{}` exceeded {}.\n\
					 The channel was created at:\n{:}\n
					 The last event was sent from:\n{:}",
					sender.name,
					sender.queue_size_warning,
					sender.creation_backtrace,
					Backtrace::force_capture(),
				);
			} else if sender.warning_fired &&
				current_pending < sender.queue_size_warning.wrapping_div(2)
			{
				sender.warning_fired = false;
				info!(
					"Channel `{}` is no longer overflowed. Number of events: {}",
					sender.name, current_pending
				);
			}

			sender.inner.try_send(event.clone()).is_ok()
		});

		if let Some(metrics) = self.metrics.as_ref() {
			for ev in &self.event_streams {
				metrics.event_in(&event, ev.name);
			}
		}
	}
}

impl fmt::Debug for OutChannels {
	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
		f.debug_struct("OutChannels")
			.field("num_channels", &self.event_streams.len())
			.finish()
	}
}

#[derive(Clone)]
struct Metrics {
	// This list is ordered alphabetically
	events_total: CounterVec<U64>,
	notifications_sizes: CounterVec<U64>,
	num_channels: GaugeVec<U64>,
}

thread_local! {
	static LABEL_BUFFER: RefCell<String> = RefCell::new(String::new());

Review comment (suggestion):
```suggestion
		if let Some(metrics) = &self.metrics {
```

Code after:
		sender.metrics = self.metrics.clone();

		if let Some(metrics) = &self.metrics {
			metrics.num_channels.with_label_values(&[sender.name]).inc();
		}

		self.event_streams.push(sender);
	}

	/// Sends an event.
	pub fn send(&mut self, event: Event) {
		self.event_streams.retain_mut(|sender| {
			let current_pending = sender.inner.len();
			if current_pending >= sender.queue_size_warning {
				if sender.warning_fired == SenderWarningState::NotFired {
					error!(
						"The number of unprocessed events in channel `{}` exceeded {}.\n\
						 The channel was created at:\n{:}\n
						 The last event was sent from:\n{:}",
						sender.name,
						sender.queue_size_warning,
						sender.creation_backtrace,
						Backtrace::force_capture(),
					);
				} else if sender.warning_fired == SenderWarningState::FiredFree {
					// We don't want to spam the logs, so we only log on debug level
					debug!(
						target: LOG_TARGET,
						"Channel `{}` is overflowed again. Number of events: {}",
						sender.name, current_pending
					);
				}
				sender.warning_fired = SenderWarningState::FiredFull;
			} else if sender.warning_fired == SenderWarningState::FiredFull &&
				current_pending < sender.queue_size_warning.wrapping_div(2)
			{
				sender.warning_fired = SenderWarningState::FiredFree;
				debug!(
					target: LOG_TARGET,
					"Channel `{}` is no longer overflowed. Number of events: {}",
					sender.name, current_pending
				);
			}

			sender.inner.try_send(event.clone()).is_ok()
		});

		if let Some(metrics) = &self.metrics {
			for ev in &self.event_streams {
				metrics.event_in(&event, ev.name);
			}
Repository: paritytech/substrate
File: bin/node/runtime/src/impls.rs (Rust)

Code before:
		approve: bool,
	) -> Result<bool, DispatchError> {
		AllianceMotion::do_vote(who, proposal, index, approve)
	}

	fn close_proposal(
		proposal_hash: Hash,
		proposal_index: ProposalIndex,
		proposal_weight_bound: Weight,
		length_bound: u32,
	) -> DispatchResultWithPostInfo {
		AllianceMotion::do_close(proposal_hash, proposal_index, proposal_weight_bound, length_bound)
	}

	fn proposal_of(proposal_hash: Hash) -> Option<RuntimeCall> {
		AllianceMotion::proposal_of(proposal_hash)
	}
}

/// An implementation of MultiAssetId that can be either Native or an asset.
#[derive(Decode, Encode, Default, MaxEncodedLen, TypeInfo, Clone, Copy, Debug)]
pub enum NativeOrAssetId<AssetId>
where
	AssetId: Ord,
{
	/// Native asset. For example, on AssetHubPolkadot this would be dot.
	#[default]
	Native,
	/// A non-native asset id.
	Asset(AssetId),
}

impl<AssetId: Ord> From<AssetId> for NativeOrAssetId<AssetId> {
	fn from(asset: AssetId) -> Self {
		Self::Asset(asset)
	}
}

impl<AssetId: Ord> Ord for NativeOrAssetId<AssetId> {
	fn cmp(&self, other: &Self) -> Ordering {
		match (self, other) {
			(Self::Native, Self::Native) => Ordering::Equal,
			(Self::Native, Self::Asset(_)) => Ordering::Less,
			(Self::Asset(_), Self::Native) => Ordering::Greater,
			(Self::Asset(id1), Self::Asset(id2)) => <AssetId as Ord>::cmp(id1, id2),
		}
	}
}
impl<AssetId: Ord> PartialOrd for NativeOrAssetId<AssetId> {
	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
		Some(<Self as Ord>::cmp(self, other))

Review comment (suggestion):
```suggestion
	/// Native asset. For example, on the Polkadot Asset Hub this would be DOT.
```

Code after:
	) -> Result<bool, DispatchError> {
		AllianceMotion::do_vote(who, proposal, index, approve)
	}

	fn close_proposal(
		proposal_hash: Hash,
		proposal_index: ProposalIndex,
		proposal_weight_bound: Weight,
		length_bound: u32,
	) -> DispatchResultWithPostInfo {
		AllianceMotion::do_close(proposal_hash, proposal_index, proposal_weight_bound, length_bound)
	}

	fn proposal_of(proposal_hash: Hash) -> Option<RuntimeCall> {
		AllianceMotion::proposal_of(proposal_hash)
	}
}

#[cfg(test)]
mod multiplier_tests {
	use frame_support::{
		dispatch::DispatchClass,
		weights::{Weight, WeightToFee},
	};
	use pallet_transaction_payment::{Multiplier, TargetedFeeAdjustment};
	use sp_runtime::{
		assert_eq_error_rate,
		traits::{Convert, One, Zero},
		BuildStorage, FixedPointNumber,
	};

	use crate::{
		constants::{currency::*, time::*},
		AdjustmentVariable, MaximumMultiplier, MinimumMultiplier, Runtime,
		RuntimeBlockWeights as BlockWeights, System, TargetBlockFullness, TransactionPayment,
	};

	fn max_normal() -> Weight {
		BlockWeights::get()
			.get(DispatchClass::Normal)
			.max_total
			.unwrap_or_else(|| BlockWeights::get().max_block)
	}

	fn min_multiplier() -> Multiplier {
		MinimumMultiplier::get()
	}

	fn target() -> Weight {
		TargetBlockFullness::get() * max_normal()
	}
Repository: paritytech/substrate
File: frame/support/src/traits/hooks.rs (Rust)

Code before:
	/// See [`Hooks::integrity_test`].
	fn integrity_test() {}
}

/// The pallet hooks trait. This is merely an umbrella trait for:
///
/// - [`OnInitialize`]
/// - [`OnFinalize`]
/// - [`OnRuntimeUpgrade`]
/// - [`OffchainWorker`]
/// - [`OnIdle`]
/// - [`IntegrityTest`]
///
/// ## Ordering
///
/// For all hooks, except [`OnIdle`] the order of execution is derived from how the pallets are
/// ordered in [`crate::construct_runtime`].
///
/// ## Summary
///
/// In short, the following diagram shows the
pub trait Hooks<BlockNumber> {
	/// Block initialization hook. This is called at the very beginning of block execution.
	///
	/// Must return the non-negotiable weight of both itself, and whatever [`on_finalize`] wishes to
	/// consumed.
	///
	/// The weight returned by this is treated as [`DispatchClass::Mandatory`], meaning that it MUST
	/// BE EXECUTED. This implies that this hook should only ever be used if the logic in it must
	/// happen under any circumstance. For all similar use cases where a delay is acceptable, use
	/// [`poll`].
	///
	/// NOTE: This function is called BEFORE ANY extrinsic in a block is applied, including inherent
	/// extrinsics. Hence for instance, if you runtime includes `pallet-timestamp`, the `timestamp`
	/// is not yet up to date at this point.
	fn on_initialize(_n: BlockNumber) -> Weight {
		Weight::zero()
	}

	/// Similar to [`on_initialize`], but:
	///
	/// 1. is called after [`on_initialize`] of all pallets is done executing,
	/// 2. execution on *each block* is not guaranteed
	///
	/// The latter point is usually correlated with the block resources being available, for example
	/// `Weight` leftover.
	fn poll(_n: BlockNumber) -> Weight {
		Weight::zero()
	}

	/// Block finalization hook. This is called at the very end of block execution.

Review comment (suggestion):
```suggestion
	/// Must return the non-negotiable weight of both itself and whatever [`on_finalize`] wishes to
	/// consume.
```

Code after:
	/// See [`Hooks::integrity_test`].
	fn integrity_test() {}
}

#[cfg_attr(doc, aquamarine::aquamarine)]
/// The pallet hooks trait. This is merely an umbrella trait for:
///
/// - [`OnInitialize`]
/// - [`OnFinalize`]
/// - [`OnRuntimeUpgrade`]
/// - [`crate::traits::misc::OffchainWorker`]
/// - [`OnIdle`]
/// - [`IntegrityTest`]
///
/// ## Ordering
///
/// For all hooks, except [`OnIdle`] the order of execution is derived from how the pallets are
/// ordered in [`crate::construct_runtime`].
///
/// ## Summary
///
/// In short, the following diagram shows the flow of hooks in a pallet
///
/// ```mermaid
/// graph LR
/// 	Optional --> BeforeExtrinsics
/// 	BeforeExtrinsics --> Extrinsics
/// 	Extrinsics --> AfterExtrinsics
/// 	subgraph Optional
/// 	OnRuntimeUpgrade
/// end
///
/// subgraph BeforeExtrinsics
/// 	OnInitialize
/// end
///
/// subgraph Extrinsics
/// 	direction TB
/// 	Inherent1
/// 	Inherent2
/// 	Extrinsic1
/// 	Extrinsic2
///
/// 	Inherent1 --> Inherent2
/// 	Inherent2 --> Extrinsic1
/// 	Extrinsic1 --> Extrinsic2
/// end
///
/// subgraph AfterExtrinsics
/// 	OnIdle
/// 	OnFinalize
Repository: paritytech/substrate
File: frame/uniques/src/functions.rs (Rust)

Code before:
				price: *price,
				whitelisted_buyer,
			});
		} else {
			ItemPriceOf::<T, I>::remove(&collection, &item);
			Self::deposit_event(Event::ItemPriceRemoved { collection, item });
		}

		Ok(())
	}

	/// Buy an item from a collection.
	///
	/// # Errors
	/// This function returns an error in the following cases:
	/// - The item or collection does not exist (`UnknownItem` or `UnknownCollection`).
	/// - The buyer is the current owner of the item (`NoPermission`).
	/// - The item is not for sale (`NotForSale`).
	/// - The bid price is lower than the item's sale price (`BidTooLow`).
	/// - The item is set to be sold only to a specific buyer, and the provided buyer is not the
	///   whitelisted buyer (`NoPermission`).
	/// - The currency transfer between the buyer and the owner fails for any reason.
	///
	/// # Returns
	/// Returns `Ok(())` if the purchase is successful, otherwise, it returns an error indicating the
	/// reason for failure.
	pub fn do_buy_item(
		collection: T::CollectionId,
		item: T::ItemId,
		buyer: T::AccountId,
		bid_price: ItemPrice<T, I>,
	) -> DispatchResult {
		let details = Item::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::UnknownItem)?;
		ensure!(details.owner != buyer, Error::<T, I>::NoPermission);

		let price_info =
			ItemPriceOf::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::NotForSale)?;

		ensure!(bid_price >= price_info.0, Error::<T, I>::BidTooLow);

		if let Some(only_buyer) = price_info.1 {
			ensure!(only_buyer == buyer, Error::<T, I>::NoPermission);
		}

		T::Currency::transfer(
			&buyer,
			&details.owner,
			price_info.0,
			ExistenceRequirement::KeepAlive,
		)?;


Review comment (bug):
I think this is a bit superfluous, since the `Result` type already indicates that it could fail.

Code after:
	/// - The bid price is lower than the item's sale price
	///   ([`BidTooLow`](crate::Error::BidTooLow)).
	/// - The item is set to be sold only to a specific buyer, and the provided buyer is not the
	///   whitelisted buyer ([`NoPermission`](crate::Error::NoPermission)).
	/// - The currency transfer between the buyer and the owner fails for any reason.
	pub fn do_buy_item(
		collection: T::CollectionId,
		item: T::ItemId,
		buyer: T::AccountId,
		bid_price: ItemPrice<T, I>,
	) -> DispatchResult {
		let details = Item::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::UnknownItem)?;
		ensure!(details.owner != buyer, Error::<T, I>::NoPermission);

		let price_info =
			ItemPriceOf::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::NotForSale)?;

		ensure!(bid_price >= price_info.0, Error::<T, I>::BidTooLow);

		if let Some(only_buyer) = price_info.1 {
			ensure!(only_buyer == buyer, Error::<T, I>::NoPermission);
		}

		T::Currency::transfer(
			&buyer,
			&details.owner,
			price_info.0,
			ExistenceRequirement::KeepAlive,
		)?;

		let old_owner = details.owner.clone();

		Self::do_transfer(collection.clone(), item, buyer.clone(), |_, _| Ok(()))?;

		Self::deposit_event(Event::ItemBought {
			collection,
			item,
			price: price_info.0,
			seller: old_owner,
			buyer,
		});

		Ok(())
	}
}
Repository: paritytech/substrate
File: client/rpc-spec-v2/src/chain_head/chain_head_storage.rs (Rust)

Code before:

		QueryIterResult::Ok(ret)
	}

	/// Generate the block events for the `chainHead_storage` method.
	pub fn generate_events(
		&self,
		mut sink: SubscriptionSink,
		hash: Block::Hash,
		items: Vec<StorageQuery<StorageKey>>,
		child_key: Option<ChildInfo>,
	) {
		if let Some(child_key) = child_key.as_ref() {
			if !is_key_queryable(child_key.storage_key()) {
				let _ = sink.send(&ChainHeadStorageEvent::<String>::Done);
				return
			}
		}

		let mut storage_results = Vec::with_capacity(items.len());
		for item in items {
			if !is_key_queryable(&item.key.0) {
				continue
			}

			match item.queue_type {
				StorageQueryType::Value => {
					match self.query_storage_value(hash, &item.key, child_key.as_ref()) {
						Ok(Some(value)) => storage_results.push(value),
						Ok(None) => continue,
						Err(err) => {
							let _ = sink.send(&err);
							return
						},
					}
				},
				StorageQueryType::Hash =>
					match self.query_storage_hash(hash, &item.key, child_key.as_ref()) {
						Ok(Some(value)) => storage_results.push(value),
						Ok(None) => continue,
						Err(err) => {
							let _ = sink.send(&err);
							return
						},
					},
				StorageQueryType::DescendantsValues => match self.query_storage_iter(
					hash,
					&item.key,
					child_key.as_ref(),
					IterQueryType::Value,
				) {

Review comment (suggestion):
Probably an oversight from the last PR, but I think this should be `query_type` instead of `queue_type`.

Code after:
		&self,
		mut sink: SubscriptionSink,
		hash: Block::Hash,
		items: Vec<StorageQuery<StorageKey>>,
		child_key: Option<ChildInfo>,
	) {
		if let Some(child_key) = child_key.as_ref() {
			if !is_key_queryable(child_key.storage_key()) {
				let _ = sink.send(&ChainHeadStorageEvent::<String>::Done);
				return
			}
		}

		let mut storage_results = Vec::with_capacity(items.len());
		for item in items {
			if !is_key_queryable(&item.key.0) {
				continue
			}

			match item.query_type {
				StorageQueryType::Value => {
					match self.query_storage_value(hash, &item.key, child_key.as_ref()) {
						Ok(Some(value)) => storage_results.push(value),
						Ok(None) => continue,
						Err(err) => {
							let _ = sink.send(&err);
							return
						},
					}
				},
				StorageQueryType::Hash =>
					match self.query_storage_hash(hash, &item.key, child_key.as_ref()) {
						Ok(Some(value)) => storage_results.push(value),
						Ok(None) => continue,
						Err(err) => {
							let _ = sink.send(&err);
							return
						},
					},
				StorageQueryType::DescendantsValues => match self.query_storage_iter(
					hash,
					&item.key,
					child_key.as_ref(),
					IterQueryType::Value,
				) {
					Ok(values) => storage_results.extend(values),
					Err(err) => {
						let _ = sink.send(&err);
						return
					},
				},
Repository: paritytech/substrate
File: client/rpc-spec-v2/src/chain_head/chain_head_storage.rs (Rust)

Code before:
		let mut keys_iter = if let Some(child_key) = child_key {
			self.client.child_storage_keys(hash, child_key.to_owned(), Some(key), None)
		} else {
			self.client.storage_keys(hash, Some(key), None)
		}
		.map_err(|err| {
			ChainHeadStorageEvent::<String>::Error(ErrorEvent { error: err.to_string() })
		})?;

		let mut ret = Vec::with_capacity(MAX_ITER_ITEMS);
		let mut maximum_iters = MAX_ITER_ITEMS;
		while let Some(key) = keys_iter.next() {
			if maximum_iters > 0 {
				maximum_iters -= 1;
			}

			let result = match ty {
				IterQueryType::Value => self.query_storage_value(hash, &key, child_key),
				IterQueryType::Hash => self.query_storage_hash(hash, &key, child_key),
			}?;

			let Some(result) = result else {
				continue
			};

			ret.push(result);
		}

		QueryIterResult::Ok(ret)
	}

	/// Generate the block events for the `chainHead_storage` method.
	pub fn generate_events(
		&self,
		mut sink: SubscriptionSink,
		hash: Block::Hash,
		items: Vec<StorageQuery<StorageKey>>,
		child_key: Option<ChildInfo>,
	) {
		if let Some(child_key) = child_key.as_ref() {
			if !is_key_queryable(child_key.storage_key()) {
				let _ = sink.send(&ChainHeadStorageEvent::<String>::Done);
				return
			}
		}

		let mut storage_results = Vec::with_capacity(items.len());
		for item in items {
			if !is_key_queryable(&item.key.0) {
				continue
			}

Review comment (suggestion):

```suggestion
			if let Some(result) = result {
				ret.push(result)
			};
```

Code after:
		let keys_iter = if let Some(child_key) = child_key {
			self.client.child_storage_keys(hash, child_key.to_owned(), Some(key), None)
		} else {
			self.client.storage_keys(hash, Some(key), None)
		}
		.map_err(|err| {
			ChainHeadStorageEvent::<String>::Error(ErrorEvent { error: err.to_string() })
		})?;

		let mut ret = Vec::with_capacity(MAX_ITER_ITEMS);
		let mut keys_iter = keys_iter.take(MAX_ITER_ITEMS);
		while let Some(key) = keys_iter.next() {
			let result = match ty {
				IterQueryType::Value => self.query_storage_value(hash, &key, child_key),
				IterQueryType::Hash => self.query_storage_hash(hash, &key, child_key),
			}?;

			if let Some(result) = result {
				ret.push(result);
			}
		}

		QueryIterResult::Ok(ret)
	}

	/// Generate the block events for the `chainHead_storage` method.
	pub fn generate_events(
		&self,
		mut sink: SubscriptionSink,
		hash: Block::Hash,
		items: Vec<StorageQuery<StorageKey>>,
		child_key: Option<ChildInfo>,
	) {
		if let Some(child_key) = child_key.as_ref() {
			if !is_key_queryable(child_key.storage_key()) {
				let _ = sink.send(&ChainHeadStorageEvent::<String>::Done);
				return
			}
		}

		let mut storage_results = Vec::with_capacity(items.len());
		for item in items {
			if !is_key_queryable(&item.key.0) {
				continue
			}

			match item.query_type {
				StorageQueryType::Value => {
					match self.query_storage_value(hash, &item.key, child_key.as_ref()) {
						Ok(Some(value)) => storage_results.push(value),
						Ok(None) => continue,
Repository: paritytech/substrate
File: client/rpc-spec-v2/src/chain_head/chain_head_storage.rs (Rust)

Code before:
			})
	}

	/// Handle iterating over (key, value) or (key, hash) pairs.
	fn query_storage_iter(
		&self,
		hash: Block::Hash,
		key: &StorageKey,
		child_key: Option<&ChildInfo>,
		ty: IterQueryType,
	) -> QueryIterResult {
		let mut keys_iter = if let Some(child_key) = child_key {
			self.client.child_storage_keys(hash, child_key.to_owned(), Some(key), None)
		} else {
			self.client.storage_keys(hash, Some(key), None)
		}
		.map_err(|err| {
			ChainHeadStorageEvent::<String>::Error(ErrorEvent { error: err.to_string() })
		})?;

		let mut ret = Vec::with_capacity(MAX_ITER_ITEMS);
		let mut maximum_iters = MAX_ITER_ITEMS;
		while let Some(key) = keys_iter.next() {
			if maximum_iters > 0 {
				maximum_iters -= 1;
			}

			let result = match ty {
				IterQueryType::Value => self.query_storage_value(hash, &key, child_key),
				IterQueryType::Hash => self.query_storage_hash(hash, &key, child_key),
			}?;

			let Some(result) = result else {
				continue
			};

			ret.push(result);
		}

		QueryIterResult::Ok(ret)
	}

	/// Generate the block events for the `chainHead_storage` method.
	pub fn generate_events(
		&self,
		mut sink: SubscriptionSink,
		hash: Block::Hash,
		items: Vec<StorageQuery<StorageKey>>,
		child_key: Option<ChildInfo>,
	) {
		if let Some(child_key) = child_key.as_ref() {

Review comment (suggestion):
We would give out more than 10 items here, `maximum_iters` is counting down but does not abort. How about something like this?
```suggestion
		keys_iter = keys_iter.take(MAX_ITER_ITEMS);
		while let Some(key) = keys_iter.next() {
```

Code after:
			})
	}

	/// Handle iterating over (key, value) or (key, hash) pairs.
	fn query_storage_iter(
		&self,
		hash: Block::Hash,
		key: &StorageKey,
		child_key: Option<&ChildInfo>,
		ty: IterQueryType,
	) -> QueryIterResult {
		let keys_iter = if let Some(child_key) = child_key {
			self.client.child_storage_keys(hash, child_key.to_owned(), Some(key), None)
		} else {
			self.client.storage_keys(hash, Some(key), None)
		}
		.map_err(|err| {
			ChainHeadStorageEvent::<String>::Error(ErrorEvent { error: err.to_string() })
		})?;

		let mut ret = Vec::with_capacity(MAX_ITER_ITEMS);
		let mut keys_iter = keys_iter.take(MAX_ITER_ITEMS);
		while let Some(key) = keys_iter.next() {
			let result = match ty {
				IterQueryType::Value => self.query_storage_value(hash, &key, child_key),
				IterQueryType::Hash => self.query_storage_hash(hash, &key, child_key),
			}?;

			if let Some(result) = result {
				ret.push(result);
			}
		}

		QueryIterResult::Ok(ret)
	}

	/// Generate the block events for the `chainHead_storage` method.
	pub fn generate_events(
		&self,
		mut sink: SubscriptionSink,
		hash: Block::Hash,
		items: Vec<StorageQuery<StorageKey>>,
		child_key: Option<ChildInfo>,
	) {
		if let Some(child_key) = child_key.as_ref() {
			if !is_key_queryable(child_key.storage_key()) {
				let _ = sink.send(&ChainHeadStorageEvent::<String>::Done);
				return
			}
		}