ronantakizawa/codereview-bench huggingface dataset:
Repository: Moya/Moya
File: Sources/Moya/Plugins/AccessTokenPlugin.swift (Swift)

Code before:
// MARK: - AccessTokenAuthorizable

/// A protocol for controlling the behavior of `AccessTokenPlugin`.
public protocol AccessTokenAuthorizable {

    /// Represents the authorization header to use for requests.
    var authorizationType: AuthorizationType { get }
}

// MARK: - AuthorizationType

/// An enum representing the header to use with an `AccessTokenPlugin`
public enum AuthorizationType {
    /// No header.
    case none

    /// The `"Basic"` header.
    case basic

    /// The `"Bearer"` header.
    case bearer
    
    /// Custom header implementation.
    case custom(String)
    
    public var value: String {
        
        switch self {
        case .none: return ""
        case .basic, .bearer: return String(describing: self).capitalized
        case .custom(let custom): return custom
        }
    }
}

// MARK: - AccessTokenPlugin

/**
 A plugin for adding basic or bearer-type authorization headers to requests. Example:

 ```
 Authorization: Basic <token>
 Authorization: Bearer <token>
 Authorization: <Сustom> <token>
 ```

*/
public struct AccessTokenPlugin: PluginType {

    /// A closure returning the access token to be applied in the header.
    public let tokenClosure: () -> String

Review comment (suggestion):
~I think this makes more sense to be optional and return `nil` for `.none`. We just `break` on `.none` so having a non-nil value doesn't save us much. It just makes it more confusing for a user that they are receiving an empty string.~

Code after:
// MARK: - AccessTokenAuthorizable

/// A protocol for controlling the behavior of `AccessTokenPlugin`.
public protocol AccessTokenAuthorizable {

    /// Represents the authorization header to use for requests.
    var authorizationType: AuthorizationType { get }
}

// MARK: - AuthorizationType

/// An enum representing the header to use with an `AccessTokenPlugin`
public enum AuthorizationType {
    /// No header.
    case none

    /// The `"Basic"` header.
    case basic

    /// The `"Bearer"` header.
    case bearer

    /// Custom header implementation.
    case custom(String)

    public var value: String? {
        switch self {
        case .none: return nil
        case .basic: return "Basic"
        case .bearer: return "Bearer"
        case .custom(let customValue): return customValue
        }
    }
}

// MARK: - AccessTokenPlugin

/**
 A plugin for adding basic or bearer-type authorization headers to requests. Example:

 ```
 Authorization: Basic <token>
 Authorization: Bearer <token>
 Authorization: <Сustom> <token>
 ```

*/
public struct AccessTokenPlugin: PluginType {

    /// A closure returning the access token to be applied in the header.
    public let tokenClosure: () -> String
Repository: MystenLabs/sui
File: sui-execution/latest/sui-move-natives/src/funds_accumulator.rs (Rust)

Code before:

    let value = args.pop_back().unwrap().value_as::<U256>().unwrap();
    let recipient = args
        .pop_back()
        .unwrap()
        .value_as::<AccountAddress>()
        .unwrap();
    let accumulator: ObjectID = args
        .pop_back()
        .unwrap()
        .value_as::<AccountAddress>()
        .unwrap()
        .into();

    // TODO this will need to look at the layout of T when this is not guaranteed to be a Balance
    let Ok(amount): Result<u64, _> = value.try_into() else {
        return Ok(NativeResult::err(context.gas_used(), E_OVERFLOW));
    };

    let cost = context.gas_used();

    let obj_runtime: &mut ObjectRuntime = context.extensions_mut().get_mut()?;

    if !obj_runtime.protocol_config.enable_accumulators() {
        return Ok(NativeResult::err(cost, E_ADDRESS_BALANCE_NOT_ENABLED));
    }

    obj_runtime.emit_accumulator_event(
        accumulator,
        MoveAccumulatorAction::Split,
        recipient,
        ty_tag,
        MoveAccumulatorValue::U64(amount),
    )?;
    // TODO this will need to look at the layout of T when this is not guaranteed to be a Balance
    let withdrawn = Value::struct_(Struct::pack(vec![Value::u64(amount)]));
    Ok(NativeResult::ok(context.gas_used(), smallvec![withdrawn]))
}


Review comment (refactor):
Can you move this above the cast into u64? That is technically logic specific to the implementation here that we should not yet expose 

Code after:

    let value = args.pop_back().unwrap().value_as::<U256>().unwrap();
    let recipient = args
        .pop_back()
        .unwrap()
        .value_as::<AccountAddress>()
        .unwrap();
    let accumulator: ObjectID = args
        .pop_back()
        .unwrap()
        .value_as::<AccountAddress>()
        .unwrap()
        .into();

    // TODO this will need to look at the layout of T when this is not guaranteed to be a Balance
    let Ok(amount): Result<u64, _> = value.try_into() else {
        return Ok(NativeResult::err(context.gas_used(), E_OVERFLOW));
    };

    let obj_runtime: &mut ObjectRuntime = context.extensions_mut().get_mut()?;
    obj_runtime.emit_accumulator_event(
        accumulator,
        MoveAccumulatorAction::Split,
        recipient,
        ty_tag,
        MoveAccumulatorValue::U64(amount),
    )?;
    // TODO this will need to look at the layout of T when this is not guaranteed to be a Balance
    let withdrawn = Value::struct_(Struct::pack(vec![Value::u64(amount)]));
    Ok(NativeResult::ok(context.gas_used(), smallvec![withdrawn]))
}
Repository: MystenLabs/sui
File: crates/sui/src/client_commands.rs (Rust)

Code before:
    let chain_id = read_api.get_chain_identifier().await?;

    check_protocol_version_and_warn(read_api).await?;
    let package_path =
        package_path
            .canonicalize()
            .map_err(|e| SuiErrorKind::ModulePublishFailure {
                error: format!("Failed to canonicalize package path: {}", e),
            })?;

    let compiled_package = compile_package(
        read_api,
        root_package,
        build_config.clone(),
        &package_path,
        with_unpublished_dependencies,
    )
    .await;

    let compiled_package = compiled_package?;

    let out_dir = build_config
        .install_dir
        .clone()
        .unwrap_or(root_package.package_path().to_path_buf());
    root_package.save_lockfile_to_disk(&out_dir)?;
    let compiled_modules = compiled_package.get_package_bytes(with_unpublished_dependencies);
    let dep_ids = compiled_package.get_published_dependencies_ids();

    let tx_kind = client
        .transaction_builder()
        .publish_tx_kind(sender, compiled_modules, dep_ids)
        .await?;

    let gas_payment = client
        .transaction_builder()
        .input_refs(&payment.gas)
        .await?;

    let result = dry_run_or_execute_or_serialize(
        sender,
        tx_kind,
        context,
        gas_payment,
        gas_data,
        processing,
    )
    .await?;

    let response = if let SuiClientCommandResult::TransactionBlock(ref tx) = result {
        tx

Review comment (suggestion):
I am not 100% sure this is correct. Have you checked with the current system, if you pass the `install-dir` if the lockfile gets written there? I recall the `install-dir` is only for the `build` folder.

Code after:
        package_path
            .canonicalize()
            .map_err(|e| SuiErrorKind::ModulePublishFailure {
                error: format!("Failed to canonicalize package path: {}", e),
            })?;

    let compiled_package = compile_package(
        read_api,
        root_package,
        build_config.clone(),
        &package_path,
        with_unpublished_dependencies,
    )
    .await;

    let compiled_package = compiled_package?;

    root_package.save_lockfile_to_disk()?;
    let compiled_modules = compiled_package.get_package_bytes(with_unpublished_dependencies);
    let dep_ids = compiled_package.get_published_dependencies_ids();

    let tx_kind = client
        .transaction_builder()
        .publish_tx_kind(sender, compiled_modules, dep_ids)
        .await?;

    let gas_payment = client
        .transaction_builder()
        .input_refs(&payment.gas)
        .await?;

    let result = dry_run_or_execute_or_serialize(
        sender,
        tx_kind,
        context,
        gas_payment,
        gas_data,
        processing,
    )
    .await?;

    let response = if let SuiClientCommandResult::TransactionBlock(ref tx) = result {
        tx
    } else {
        bail!("Error")
    };

    let publish_data = update_publication(
        &chain_id,
        LockCommand::Publish,
        response,
Repository: MystenLabs/sui
File: crates/sui-package-alt/src/sui_flavor.rs (Rust)

Code before:
    if manifest.legacy_data.is_some() {
        return Ok(());
    }

    // collect all manifest deps
    let mut dep_names = manifest
        .dependencies
        .keys()
        .map(|n| n.get_ref().to_string())
        .collect::<Vec<_>>();

    // Check "dep replacements" too.
    dep_names.extend(
        manifest
            .dep_replacements
            .values()
            .flat_map(|k| k.keys().map(|key| key.to_string()))
            .collect::<Vec<_>>(),
    );

    let legacy_names = SuiFlavor::system_deps_names_map();

    for name in dep_names {
        if legacy_names.contains_key(&name) {
            return Err(format!(
                "Dependency `{name}` is a legacy system name and cannot be used. See: https://docs.sui.io/guides/developer/sui-101/move-package-management#system-dependencies"
            ));
        }
    }

    Ok(())
}

impl Default for BuildParams {
    fn default() -> Self {
        Self {
            flavor: FLAVOR.to_string(),
            edition: EDITION.to_string(),
        }
    }
}


Review comment (suggestion):
```suggestion
                "Dependency `{name}` is a legacy system name and cannot be used. See https://docs.sui.io/guides/developer/sui-101/move-package-management#system-dependencies"
```

Code after:
    if manifest.legacy_data.is_some() {
        return Ok(());
    }

    // collect all manifest deps
    let mut dep_names = manifest
        .dependencies
        .keys()
        .map(|n| n.get_ref().to_string())
        .collect::<Vec<_>>();

    // Check "dep replacements" too.
    dep_names.extend(
        manifest
            .dep_replacements
            .values()
            .flat_map(|k| k.keys().map(|key| key.to_string()))
            .collect::<Vec<_>>(),
    );

    let legacy_names = SuiFlavor::system_deps_names_map();

    for name in dep_names {
        if legacy_names.contains_key(&name) {
            return Err(format!(
                "Dependency `{name}` is a legacy system name and cannot be used. See https://docs.sui.io/guides/developer/sui-101/move-package-management#system-dependencies"
            ));
        }
    }

    Ok(())
}

impl Default for BuildParams {
    fn default() -> Self {
        Self {
            flavor: FLAVOR.to_string(),
            edition: EDITION.to_string(),
        }
    }
}
Repository: MystenLabs/sui
File: crates/sui-indexer-alt-framework/src/pipeline/concurrent/main_reader_lo.rs (Rust)

Code before:

            let mut conn = match store.connect().await {
                Ok(conn) => conn,
                Err(e) => {
                    warn!(pipeline = H::NAME, "Failed to connect to store: {e}");
                    continue;
                }
            };

            let watermark = match conn.reader_watermark(H::NAME).await {
                // If the reader watermark is not present (either because the watermark entry does
                // not exist, or the reaer watermark is not set), we assume that pruning is not
                // enabled and checkpoints >= 0 are valid.
                Ok(watermark) => watermark.map_or(0, |wm| wm.reader_lo),
                Err(e) => {
                    warn!(pipeline = H::NAME, "Failed to get reader watermark: {e}");
                    continue;
                }
            };

            if let Some(lo) = reader_lo.get() {
                lo.store(watermark, Ordering::Relaxed);
            } else {
                reader_lo.set(AtomicU64::new(watermark)).ok();
            }

            metrics
                .latest_main_reader_lo
                .with_label_values(&[H::NAME])
                .set(watermark as i64);
        }
    })
}


Review comment (suggestion):
Does updating the metric here solve your race? I think you need to update it in the collector, after the collector has seen the new reader lo value (maybe therefore call this metric `collector_reader_lo` or something).

Code after:
                Ok(conn) => conn,
                Err(e) => {
                    warn!(pipeline = H::NAME, "Failed to connect to store: {e}");
                    continue;
                }
            };

            let watermark = match conn.reader_watermark(H::NAME).await {
                // If the reader watermark is not present (either because the watermark entry does
                // not exist, or the reaer watermark is not set), we assume that pruning is not
                // enabled and checkpoints >= 0 are valid.
                Ok(watermark) => watermark.map_or(0, |wm| wm.reader_lo),
                Err(e) => {
                    warn!(pipeline = H::NAME, "Failed to get reader watermark: {e}");
                    continue;
                }
            };

            if let Some(lo) = reader_lo.get() {
                lo.store(watermark, Ordering::Relaxed);
            } else {
                reader_lo.set(AtomicU64::new(watermark)).ok();
            }
        }
    })
}
Repository: MystenLabs/sui
File: external-crates/move/crates/move-vm-runtime/src/execution/tracing/mod.rs (Rust)

Code before:

/// Invoke a tracing operation when the `tracing` feature is enabled.
///
/// This macro provides zero-cost tracing when the `tracing` feature is disabled:
///
/// Using a macro instead of a generic function ensures that:
/// 1. No closure types are monomorphized when tracing is disabled
/// 2. No VMTracer method references appear in the binary
/// 3. The tracer parameter type can be `()` when disabled (zero-sized)
///
/// # Usage
/// ```ignore
/// trace!(run_context.tracer, |tracer| {
///     tracer.start_instruction(vtables, state, &gas_meter.remaining_gas().into())
/// });
/// ```
#[cfg(feature = "tracing")]
pub(crate) const TRACING_ENABLED: bool = true;

#[cfg(not(feature = "tracing"))]
pub(crate) const TRACING_ENABLED: bool = false;

macro_rules! trace {
    ($tracer:expr, |$param:ident| $body:expr) => {
        if crate::execution::tracing::TRACING_ENABLED {
            if let Some($param) = $tracer.as_mut() {
                $body
            }
        }
    };
}

pub(crate) use trace;


Review comment (suggestion):
Nit: ever since 1.90/Rust 2024 I believe you can rewrite this as 
```rust
if crate::execution::tracing::TRACING_ENABLED && let Some($param) = $tracer.as_mut() {
   ...
}
```

Code after:

/// Invoke a tracing operation when the `tracing` feature is enabled.
///
/// This macro provides zero-cost tracing when the `tracing` feature is disabled:
///
/// Using a macro instead of a generic function ensures that:
/// 1. No closure types are monomorphized when tracing is disabled
/// 2. No VMTracer method references appear in the binary
/// 3. The tracer parameter type can be `()` when disabled (zero-sized)
///
/// # Usage
/// ```ignore
/// trace!(run_context.tracer, |tracer| {
///     tracer.start_instruction(vtables, state, &gas_meter.remaining_gas().into())
/// });
/// ```
#[cfg(feature = "tracing")]
pub(crate) const TRACING_ENABLED: bool = true;

#[cfg(not(feature = "tracing"))]
pub(crate) const TRACING_ENABLED: bool = false;

macro_rules! trace {
    ($tracer:expr, |$param:ident| $body:expr) => {
        if crate::execution::tracing::TRACING_ENABLED && let Some($param) = $tracer.as_mut() {
            $body
        }
    };
}

pub(crate) use trace;
Repository: MystenLabs/sui
File: crates/sui-indexer-alt-e2e-tests/tests/graphql_simulate_transaction_tests.rs (Rust)

Code before:
            query($txJson: JSON!) {
                simulateTransaction(transaction: $txJson, doGasSelection: true) {
                    effects {
                        status
                        transaction {
                            gasInput {
                                gasBudget
                            }
                        }
                    }
                    error
                }
            }
        "#,
            json!({ "txJson": tx_json }),
        )
        .await
        .expect("GraphQL request failed");

    insta::assert_json_snapshot!(result.pointer("/data/simulateTransaction"), @r#"
    {
      "effects": {
        "status": "SUCCESS",
        "transaction": {
          "gasInput": {
            "gasBudget": "156976000"
          }
        }
      },
      "error": null
    }
    "#);
}


Review comment (suggestion):
Even the gas budget may change because of unrelated protocol config or gas model changes. I think it will be best to write a more "E2E" test where we take the output from GraphQL, sign it, execute it, and confirm that this succeeds and the transfer actually happens.

Code after:
    let simulate_result = graphql_cluster
        .execute_graphql(
            r#"
            query($txJson: JSON!) {
                simulateTransaction(transaction: $txJson, doGasSelection: true) {
                    effects {
                        status
                        transaction {
                            transactionBcs
                        }
                    }
                    error
                }
            }
        "#,
            json!({ "txJson": tx_json }),
        )
        .await
        .expect("GraphQL simulation request failed");

    assert_eq!(
        simulate_result.pointer("/data/simulateTransaction/effects/status"),
        Some(&json!("SUCCESS"))
    );

    // Step 2: Extract the transaction BCS, sign it, and execute
    let tx_bcs_base64 = simulate_result
        .pointer("/data/simulateTransaction/effects/transaction/transactionBcs")
        .and_then(|v| v.as_str())
        .expect("Simulation should return transactionBcs");

    let tx_bytes = Base64::decode(tx_bcs_base64).unwrap();
    let tx_data: sui_types::transaction::TransactionData = bcs::from_bytes(&tx_bytes).unwrap();

    let signed_tx = validator_cluster.sign_transaction(&tx_data).await;
    let (signed_tx_bytes, signatures) = signed_tx.to_tx_bytes_and_signatures();

    // Step 3: Execute and verify the transaction succeeds
    let execute_result = graphql_cluster
        .execute_graphql(
            r#"
            mutation($txData: Base64!, $sigs: [Base64!]!) {
                executeTransaction(transactionDataBcs: $txData, signatures: $sigs) {
                    effects { status }
                    errors
                }
            }
        "#,
            json!({
                "txData": signed_tx_bytes.encoded(),
                "sigs": signatures.iter().map(|s| s.encoded()).collect::<Vec<_>>()
Repository: MystenLabs/sui
File: crates/sui-indexer-alt-graphql/src/api/types/transaction_kind/programmable/inputs/balance_withdraw.rs (Rust)

Code before:

// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use async_graphql::{Enum, SimpleObject, Union};
use sui_types::transaction::{
    FundsWithdrawalArg as NativeFundsWithdrawalArg, Reservation as NativeReservation,
    WithdrawFrom as NativeWithdrawFrom,
};

use crate::{api::scalars::uint53::UInt53, api::types::move_type::MoveType, scope::Scope};

/// Input for withdrawing funds from an accumulator.
#[derive(SimpleObject, Clone)]
pub struct BalanceWithdraw {
    pub reservation: Option<WithdrawalReservation>,

    /// The type of the funds accumulator to withdraw from (e.g. `0x2::balance::Balance<0x2::sui::SUI>`).
    pub type_arg: Option<MoveType>,

    pub withdraw_from: Option<WithdrawFrom>,
}

/// Reservation details for a withdrawal.
#[derive(Union, Clone)]
pub enum WithdrawalReservation {
    EntireBalance(EntireBalance),
    MaxAmountU64(MaxAmountU64),
}

#[derive(SimpleObject, Clone)]
pub struct EntireBalance {
    /// Placeholder field.
    #[graphql(name = "_")]
    pub dummy: Option<bool>,
}

#[derive(SimpleObject, Clone)]
pub struct MaxAmountU64 {
    pub amount: Option<UInt53>,
}

#[derive(Enum, Copy, Clone, Eq, PartialEq)]
pub enum WithdrawFrom {

Review comment (suggestion):
```suggestion
    pub type_: Option<MoveType>,
```

Code after:
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use async_graphql::Enum;
use async_graphql::SimpleObject;
use async_graphql::Union;
use move_core_types::account_address::AccountAddress;
use sui_types::transaction::FundsWithdrawalArg as NativeFundsWithdrawalArg;
use sui_types::transaction::Reservation as NativeReservation;
use sui_types::transaction::WithdrawFrom as NativeWithdrawFrom;
use sui_types::transaction::WithdrawalTypeArg as NativeWithdrawalTypeArg;
use sui_types::type_input::StructInput;
use sui_types::type_input::TypeInput;

use crate::api::scalars::big_int::BigInt;
use crate::api::types::move_type::MoveType;
use crate::scope::Scope;

/// Input for withdrawing funds from an accumulator.
#[derive(SimpleObject, Clone)]
pub struct BalanceWithdraw {
    /// How much to withdraw from the accumulator.
    pub reservation: Option<WithdrawalReservation>,

    /// The type of the funds accumulator to withdraw from (e.g. `0x2::balance::Balance<0x2::sui::SUI>`).
    #[graphql(name = "type")]
    pub type_: Option<MoveType>,

    /// The account to withdraw funds from.
    pub withdraw_from: Option<WithdrawFrom>,
}

/// Reservation details for a withdrawal.
#[derive(Union, Clone)]
pub enum WithdrawalReservation {
    MaxAmountU64(WithdrawMaxAmountU64),
}

#[derive(SimpleObject, Clone)]
pub struct WithdrawMaxAmountU64 {
    pub amount: Option<BigInt>,
}

/// The account to withdraw funds from.