Skip to content

feat(connector): [Authorizedotnet] add network transaction id support#13156

Open
likhinbopanna wants to merge 3 commits into
mainfrom
authorizedotnet-ntid-fix
Open

feat(connector): [Authorizedotnet] add network transaction id support#13156
likhinbopanna wants to merge 3 commits into
mainfrom
authorizedotnet-ntid-fix

Conversation

@likhinbopanna

Copy link
Copy Markdown
Contributor

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

  • Add authorizedotnet to network_transaction_id_supported_connectors across all config deployments
  • Handle CardDetailsForNetworkTransactionId and MandatePayment in AuthorizeDotNet transformer payment request
  • Fix debug-build stack overflow by setting RUST_MIN_STACK at runtime before Tokio runtime creation
  • Update cypress test configs for authorizedotnet NTID flow

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

image

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible

- Add authorizedotnet to network_transaction_id_supported_connectors across all config deployments
- Handle CardDetailsForNetworkTransactionId and MandatePayment in AuthorizeDotNet transformer payment request
- Fix debug-build stack overflow by setting RUST_MIN_STACK at runtime before Tokio runtime creation
- Update cypress test configs for authorizedotnet NTID flow
@likhinbopanna likhinbopanna requested review from a team as code owners July 3, 2026 09:44
@semanticdiff-com

semanticdiff-com Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  crates/router/src/bin/router.rs  11% smaller
  crates/hyperswitch_connectors/src/connectors/authorizedotnet/transformers.rs  6% smaller
  config/config.example.toml Unsupported file format
  config/deployments/integration_test.toml Unsupported file format
  config/deployments/production.toml Unsupported file format
  config/deployments/sandbox.toml Unsupported file format
  config/development.toml Unsupported file format
  config/docker_compose.toml Unsupported file format
  crates/router/build.rs  0% smaller
  cypress-tests/cypress/e2e/configs/Payment/Utils.js  0% smaller

@likhinbopanna likhinbopanna added the S-test-ready Status: This PR is ready for cypress-tests label Jul 3, 2026
@XyneSpaces

Copy link
Copy Markdown

PLACEHOLDER_COMMENT_FOR_RETRIEVAL

@github-actions github-actions Bot removed the S-test-ready Status: This PR is ready for cypress-tests label Jul 3, 2026
@XyneSpaces

Copy link
Copy Markdown

Code Review Findings

Overall Assessment

This PR adds Network Transaction ID support for Authorizedotnet and includes a debug build stack overflow fix. The changes are generally well-structured, but there are a few items to address:

Findings

1. [should-fix] Missing SAFETY comment for runtime environment variable manipulation

In crates/router/src/bin/router.rs, the code sets RUST_MIN_STACK at runtime before spawning the Tokio runtime. While the extensive inline comment explains why, there should be a // SAFETY: comment explicitly noting that this is safe because:

  • It's done before any threads are spawned
  • The environment variable is only read by std::thread spawning, which hasn't happened yet
// SAFETY: Setting RUST_MIN_STACK before any thread creation is safe as std::thread 
// reads this env var only at spawn time. We set this before building the Tokio runtime.
#[cfg(debug_assertions)]
if std::env::var_os("RUST_MIN_STACK").is_none() {
    std::env::set_var("RUST_MIN_STACK", "67108864"); // 64 MiB
}

2. [nit] MandatePayment returning None for payment details

In transformers.rs, the MandatePayment variant now returns None for the payment field. Please confirm this is intentional - for mandate payments, Authorizedotnet may still require some payment reference. If this is correct, consider adding a comment explaining why None is appropriate for mandates.

3. [nit] Import reordering in Utils.js

The alphabetization of imports (bitpay/billwerk, mifinity placement) is good housekeeping, but ideally should be a separate commit or PR to keep the functional changes isolated.

Approved with minor suggestions

The functional changes look correct. The NTID support implementation follows the existing patterns, and the stack overflow fix addresses a real debug-build issue. Please consider adding the SAFETY comment before merge.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Unreachable arm uses generic error instead of explicit handling

In the payment match arm at line 949, PaymentMethodData::MandatePayment maps to None. Since this is an explicit match arm for a variant moved from the wildcard, this branch should either:

  1. Return a specific NotImplemented error explaining that MandatePayment is not supported for this flow
  2. Include a comment explaining why None is the correct handling

The current silent None makes debugging difficult when this variant is unexpectedly encountered.

PaymentMethodData::MandatePayment => {
    // Explicitly not supported for this Authorizedotnet flow
    return Err(IntegrationError::NotImplemented(
        "MandatePayment not supported for Authorizedotnet".to_string(),
        Default::default(),
    ))?;
}

@XyneSpaces

Copy link
Copy Markdown

⚠️ Stack size configuration moved to runtime but lacks upper bound check

The RUST_MIN_STACK is now set at runtime to 64 MiB when not present. However, there's no validation that an explicitly set value is reasonable. A maliciously high value could cause resource exhaustion.

Consider adding a maximum bound check:

if let Some(val) = std::env::var_os("RUST_MIN_STACK") {
    if let Ok(bytes) = val.to_string_lossy().parse::<usize>() {
        if bytes > 512 * 1024 * 1024 { // 512 MiB max
            eprintln!("Warning: RUST_MIN_STACK exceeds maximum allowed, using default");
            std::env::set_var("RUST_MIN_STACK", "67108864");
        }
    }
}

@likhinbopanna likhinbopanna force-pushed the authorizedotnet-ntid-fix branch from 9f74c7e to ff1b36f Compare July 3, 2026 19:49
@likhinbopanna likhinbopanna added the S-test-ready Status: This PR is ready for cypress-tests label Jul 3, 2026
@XyneSpaces

Copy link
Copy Markdown

Automated Code Review Results

Team: Connector

Findings Summary

✅ Well-Rounded Feature Implementation
This PR adds Network Transaction ID support for Authorizedotnet across config, transformer logic, and tests. Also includes a valuable stack overflow fix.

Detailed Review

1. Connector Configuration (6 config files)
Adding authorizedotnet to the NTID connector list is straightforward. The consistency across environments (example, integration_test, production, sandbox, development, docker_compose) is good.

2. Transformer Changes (transformers.rs)
The new match arm for CardDetailsForNetworkTransactionId properly extracts card details:

PaymentMethodData::CardDetailsForNetworkTransactionId(ref card_details) => {
    Some(PaymentDetails::CreditCard(CreditCardDetails {
        card_number: (*card_details.card_number).clone(),
        expiration_date: card_details.get_expiry_date_as_yyyymm("-"),
        card_code: None,
    }))
}

⚠️ Pattern Consistency Note:
The change from single-expression match to statement-style match (with explicit Some(...)) increases verbosity slightly. Consider if a helper function could reduce duplication between the Card and CardDetailsForNetworkTransactionId arms since they share identical field extraction logic.

3. Stack Overflow Fix (router/build.rsrouter.rs)
This is an excellent fix! Moving RUST_MIN_STACK from compile-time to runtime ensures:

  • All worker threads get the increased stack size
  • Debug builds won't mysteriously abort on deeply-nested async flows
  • The 64 MiB size (vs previous 11 MiB) provides ample headroom

🔍 Suggestion:
Consider adding a startup log line confirming the stack size:

#[cfg(debug_assertions)] {
    let stack_size = std::env::var("RUST_MIN_STACK").unwrap_or_default();
    logger::debug!("Worker thread stack size: {}", stack_size);
}

4. Test Updates
Removing authorizedotnet from EXCLUDE.CONNECTOR_AGNOSTIC_NTID is correct now that NTID is supported.

Overall Assessment: Clean implementation with a valuable infrastructure fix. Approved.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Missing error context in MandatePayment branch

In crates/hyperswitch_connectors/src/connectors/authorizedotnet/transformers.rs (~line 938):

PaymentMethodData::MandatePayment => None,

Returning None for MandatePayment without any context or error could silently create an invalid request. Authorize.Net requires transaction details for all payment types.

Questions:

  1. Is returning None here actually valid for NTID-based subsequent transactions?
  2. If this represents an MIT (Merchant Initiated Transaction), shouldn't there be a different request structure entirely?
  3. Should this return an error indicating MandatePayment isn't directly supported?

Fix: Add a comment explaining when None is valid, or return an appropriate error if this path shouldn't be reachable.

PaymentMethodData::MandatePayment => {
    // MandatePayment data comes from a stored mandate;
    // NTID flows don't need card details in the payment field
    None
}

@XyneSpaces

Copy link
Copy Markdown

💡 Runtime stack size modification approach

In crates/router/src/bin/router.rs (~line 42):

std::env::set_var("RUST_MIN_STACK", "67108864"); // 64 MiB

Setting stack size at runtime has limitations:

  1. It only affects threads spawned AFTER this call
  2. The main thread's stack is already allocated
  3. This affects ALL subsequently spawned threads, not just deep recursion cases

Fix: Move this configuration to:

  1. A wrapper script that sets the env var before process start, OR
  2. Documentation for operators to set in their environment, OR
  3. Compile-time configuration via .cargo/config.toml
# .cargo/config.toml
[env]
RUST_MIN_STACK = "67108864"

@XyneSpaces

Copy link
Copy Markdown

💡 Stack size increase lacks consistency with build.rs

The runtime stack size (64 MiB) is 6× larger than the previous build.rs setting (11 MiB). Additionally, std::env::set_var() is deprecated in Rust 1.86+. Consider using TOKIO_WORKER_THREAD_STACK_SIZE or Tokio's runtime builder with thread_stack_size() for cleaner configuration.

@XyneSpaces

Copy link
Copy Markdown

⚠️ CVV never sent for CardDetailsForNetworkTransactionId

The mapping explicitly sets card_code: None even if CVV is available in card_details:

Some(PaymentDetails::CreditCard(CreditCardDetails {
    card_number: ...,
    expiration_date: ...,
    card_code: None,  // CVC explicitly suppressed
}))

For NTID flows, some issuers require CVV on the first transaction. Please verify this suppression is intentional and document if NTID transactions may fail without CVV.

@XyneSpaces

Copy link
Copy Markdown

⚠️ MandatePayment silently returns None without credential-on-file data

The MandatePayment arm maps to None for the payment field:

PaymentMethodData::MandatePayment => None,

This sends empty payment data to Authorize.Net for MIT transactions. The processor will likely reject this with a generic "missing payment data" error. Either extract the mandate reference for subsequent_auth_information or return a clear NotImplemented error instead of silent failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-test-ready Status: This PR is ready for cypress-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants