feat(connector): add archipel integration flows #263
Conversation
Shubhodip900
left a comment
Shubhodip900
left a comment
There was a problem hiding this comment.
Add end to end tests and resolve the conflicts.
backend/domain_types/src/types.rs
Outdated
| let connector_meta = if value.connector_metadata.is_empty() { | ||
| None | ||
| } else { | ||
| Some(serde_json::to_value(&value.connector_metadata).unwrap_or_default()) |
There was a problem hiding this comment.
If we are checking whether the connector_metadata value is present or not then why we need unwrap_or_default? We can handle this in a better way.
There was a problem hiding this comment.
Serialization errors now bubble up instead of being silently converted to null
let connector_meta = (!value.connector_metadata.is_empty())
.then(|| serde_json::to_value(&value.connector_metadata))
.transpose()
.map_err(|_| {
error_stack::report!(ApplicationErrorResponse::InvalidDataFormat {
field_name: "connector_metadata".to_string(),
})
})?;
| ); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why we need this changes ?
There was a problem hiding this comment.
need to implement ca_certificate
There was a problem hiding this comment.
Can we handle this in better way so that we can avoid early return.
| // connector_flow::{ | ||
| // Accept, Authorize, Capture, CreateOrder, DefendDispute, PSync, RSync, Refund, | ||
| // RepeatPayment, SetupMandate, SubmitEvidence, Void, CreateSessionToken, | ||
| // }, |
There was a problem hiding this comment.
Remove the commented imports.
| // PaymentsAuthorizeData, PaymentsCaptureData, PaymentsResponseData, PaymentsSyncData, | ||
| // RefundFlowData, RefundSyncData, RefundsData, RefundsResponseData, RepeatPaymentData, | ||
| // SetupMandateRequestData, SubmitEvidenceData, SessionTokenRequestData, SessionTokenResponseData, | ||
| // }, |
siaass842
force-pushed
the
archipel-connector-integration
branch
from
1e22f0b to
0ffe865
Compare
| // State Information | ||
| optional ConnectorState state = 3; // State data for access token storage and other connector-specific state | ||
|
|
||
| map<string, string> connector_metadata = 7; |
| ACI = 1; | ||
| ADYEN = 2; | ||
| AIRWALLEX = 3; | ||
| ARCHIPEL = 90; |
| res: Response, | ||
| _event_builder: Option<&mut ConnectorEvent>, | ||
| ) -> CustomResult<ErrorResponse, errors::ConnectorError> { | ||
| // TODO: Implement proper error response parsing |
There was a problem hiding this comment.
where is the error response mapping done
backend/connector-integration/src/connectors/archipel/transformers.rs
Outdated
Show resolved
Hide resolved
| #[serde(rename_all = "camelCase")] | ||
| pub struct ArchipelOrderRequest { | ||
| amount: MinorUnit, | ||
| currency: String, |
There was a problem hiding this comment.
we can use an enum already present
| message: error_message | ||
| .description | ||
| .clone() | ||
| .unwrap_or(consts::NO_ERROR_MESSAGE.to_string()), |
There was a problem hiding this comment.
error mapping needs to be done
| ); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we handle this in better way so that we can avoid early return.
backend/connector-integration/src/connectors/archipel/transformers.rs
Outdated
Show resolved
Hide resolved
backend/connector-integration/src/connectors/archipel/transformers.rs
Outdated
Show resolved
Hide resolved
| amount: Option<i64>, | ||
| currency: Option<Currency>, | ||
| captured_amount: Option<i64>, | ||
| authorized_amount: Option<i64>, | ||
| } |
There was a problem hiding this comment.
| amount: Option<i64>, | |
| currency: Option<Currency>, | |
| captured_amount: Option<i64>, | |
| authorized_amount: Option<i64>, | |
| } | |
| amount: Option<MinorUnit>, | |
| currency: Option<Currency>, | |
| captured_amount: Option<MinorUnit>, | |
| authorized_amount: Option<MinorUnit>, | |
| } |
📝 WalkthroughWalkthroughAdds a new Archipel payment connector module with transformers, extensive flow implementations (authorize, capture, void, refund, sync, mandate, disputes, sessions, tokens), CA certificate support in connector requests, protobuf and domain updates, and configuration entries for Archipel endpoints across environments. Changes
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
config/sandbox.toml (1)
57-57: LGTM! Configuration follows established patterns.The Archipel base URL configuration is correctly added. Note: The identical URL across all environments has been flagged for verification in the development.toml review.
backend/domain_types/src/types.rs (1)
2303-2363: Connector metadata →connector_metaserialization is sound; optional simplificationThe new
connector_metawiring forPaymentServiceGetRequest(sync) looks correct and theBadRequeston JSON serialization failure is a reasonable choice for client-supplied data. If you want to reduce complexity and align with other flows (e.g. where you directly build aserde_json::Value::Objectfrom aHashMap<String, String>instead of going viaserde_json::to_value+.transpose()), you could adopt the same pattern here; that would also remove an effectively unreachable error branch. This is purely a style/consistency tweak, not a functional issue.backend/connector-integration/src/connectors/archipel/transformers.rs (4)
480-488: UseMinorUnitfor amounts inArchipelOrderResponseinstead of rawi64
ArchipelOrderResponsecurrently models amounts as:pub struct ArchipelOrderResponse { id: String, amount: Option<i64>, currency: Option<Currency>, captured_amount: Option<i64>, authorized_amount: Option<i64>, }Given the rest of the connector uses
MinorUnitfor monetary values, it would be more consistent and type‑safe to store these asOption<MinorUnit>instead ofOption<i64>. That matches the common pattern in other connectors and avoids accidental unit mismatches.This also aligns with the earlier suggestion noted in prior reviews.
1121-1143: Archipel‑specific error mapping looks goodThe
From<ArchipelErrorMessageWithHttpCode> for ErrorResponseimplementation correctly:
- propagates HTTP status,
- maps Archipel error code and description into
code,message, andreason,- falls back to
NO_ERROR_MESSAGEwhen description is missing.This addresses the earlier feedback about adding proper error response mapping for Archipel.
83-111: Removeprint!debugging around connector metadata and use structured, redacted loggingBoth
to_connector_meta(Lines 83–111) andArchipelRefundRequest::try_from(Lines 1211–1213) useprint!to dump full connector metadata. This is unsafe for production: these values can contain tenant‑specific URLs, IDs, and potentially secrets, and printing them directly to stdout is hard to control or redact.Suggested changes:
- Remove these
print!calls entirely, or- Replace them with
tracing::debug!/tracing::info!using appropriately redacted fields if you truly need them for diagnostics.While touching this code, you could also consider using
change_contextwhen mappingserde_json::from_strfailures into_connector_metaso that parse errors carry clearer context instead of being flattened.Also applies to: 1211-1213
1188-1222: Refund metadata handling: avoidprint!and consider reusingto_connector_metasemantics
ArchipelRefundRequest::try_fromprints the entireconnector_metadataand then callsArchipelConfigData::try_from(&item.router_data.request.connector_metadata)?;.Two points:
- As with
to_connector_meta, theprint!leaks potentially sensitive configuration and should be removed or replaced with structured, redacted logging.- Since
ArchipelConfigData::try_fromalready centralizes how metadata is parsed (string vs object withconnector_meta_data), reusing that consistently across flows keeps refund behavior aligned with authorize/void/setup‑mandate. If there are refund‑specific formats, it may be cleaner to document them explicitly into_connector_metarather than sprinkling assumptions across flows.backend/connector-integration/src/connectors/archipel.rs (1)
683-699: Removeprintln!/print!debug statements and avoid leaking connector metadataThere are raw
println!/print!calls in:
- Authorize
get_url(Line 691) and- PSync
get_url(Lines 733–735),which will emit connector metadata (including
connector_meta) directly to stdout. This is noisy in production and can leak sensitive information.Please remove these and, if you need diagnostics, use structured logging (
tracing::debug!/tracing::info!) with appropriate redaction, or rely on existing connector logs.Also applies to: 731-751
🧹 Nitpick comments (6)
backend/external-services/src/service.rs (1)
727-773: Clarify mutual‑TLS behavior and tighten CA‑based client constructionThe branching is clear, but a few details are worth tightening up:
- When both
client_certificateandclient_certificate_keyare present you immediately fall back toget_base_client, i.e. there is still no actual mutual TLS. That preserves previous behavior but is surprising given the parameter names. Consider either:
- Implementing mutual TLS here using the existing (commented) helpers, or
- Returning a configuration error (or at least emitting a structured log/TODO) instead of silently ignoring the certs.
- If only one of
client_certificateorclient_certificate_keyis set, the code falls through to the default client with no log. Emitting a warning in this misconfiguration case would make debugging easier.- The CA path largely duplicates
load_custom_ca_certificate_from_contentand additionally calls.use_rustls_tls(), which means this branch may use a different TLS backend than the base clients. If that difference is not intentional, consider:
- Refactoring to reuse
load_custom_ca_certificate_from_content(or a small shared helper) for adding root CAs, and- Aligning TLS backend usage between the CA and non‑CA paths, or documenting why the CA path must be rustls‑only.
These are all behavioral clarifications / polish rather than blockers, but tightening them now will avoid surprises when mTLS or connector‑specific CAs are rolled out more broadly.
backend/grpc-api-types/proto/payment.proto (1)
1261-1262: Add documentation comment for the newconnector_metadatafield.The field is correctly typed and numbered, and its presence is consistent with similar fields in
PaymentServiceAuthorizeResponse(line 1221),PaymentServiceVoidRequest(line 1334), andPaymentServiceCaptureRequest(line 1379). However, unlike most fields in this proto file, it lacks an explanatory comment describing its purpose. Adding a brief comment would improve consistency and clarity.For example:
// Connector-specific metadata to be used during payment synchronization map<string, string> connector_metadata = 7;backend/domain_types/src/types.rs (1)
94-129: Archipel connector field wiring looks correct; verify config coverageThe new
pub archipel: ConnectorParamsfield is consistent with other connectors and will deserialize from the same config shape. Because this is a non-optional field on a deserialized struct, make sure all deployed configs (all envs) define anarchipelblock; otherwise startup/deserialization can fail when rolling this out.backend/connector-integration/src/connectors/archipel.rs (2)
640-657:build_error_responseis a generic stub despite having Archipel‑specific error mappingYou now have a dedicated
ArchipelErrorMessageWithHttpCode -> ErrorResponsemapping intransformers.rs, and all happy‑path error cases from Archipel responses are handled there. Thisbuild_error_responsestill drops the response body and returnsNO_ERROR_CODE/NO_ERROR_MESSAGE, which will be used for transport/parse failures.Consider either:
- delegating to a connector‑specific parser (parsing the body into
ArchipelErrorMessagewhen possible), or- at least enriching
reason/messagewith the raw body to aid debugging, instead of the generic placeholders.This will make error reporting more actionable if we ever fall back to
build_error_response.
731-751: PSyncconnector_metaparsing is stringly and fragile; consider simplifyingPSync
get_urlassumesconnector_metais:
- present,
- an object with a
"connector_meta_data"field,- whose value is a JSON string encoding
ArchipelTransactionMetadata.This double‑encoding (
Value→ nested string →serde_json::from_str) is brittle and tightly couples URL building to a specific metadata shape. A simpler and less error‑prone approach would be to:
- store
ArchipelTransactionMetadatadirectly as aValue(or dedicated metadata type), and- deserialize via
serde_json::from_value/ an internal helper, instead of going through a nested string.Also, using
MissingConnectorTransactionIDwhenconnector_metais absent conflates two different issues (missing metadata vs missing transaction ID). A more precise error (e.g.,NoConnectorMetaDataorMissingRequiredField { field_name: "connector_meta" }) would make failures easier to debug.backend/connector-integration/src/connectors/archipel/transformers.rs (1)
296-334: Avoidserde_json::to_string(..).unwrap_or_default()to derive PAN for issuer detectionIn
ArchipelCard::try_fromyou compute:let raw_card = serde_json::to_string(&ccard.card_number.0) .unwrap_or_default() .trim_matches('"') .to_string(); let card_issuer = domain_types::utils::get_card_issuer(&raw_card).ok();Concerns:
unwrap_or_default()silently turns any serialization failure into an empty string, which then yieldscard_issuer = NoneandArchipelCardScheme::Unknown, hiding real issues.- Going through JSON and trimming quotes is brittle; it depends on how the secret type chooses to serialize.
A safer approach is to:
- use the masking crate’s expose/peek API (or an equivalent helper on
RawCardNumber<T>) to get the underlying PAN as a string, and- propagate serialization/extraction errors instead of defaulting to empty.
This keeps issuer detection deterministic and avoids silently masking real data issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/connector-integration/src/connectors.rs(1 hunks)backend/connector-integration/src/connectors/archipel.rs(1 hunks)backend/connector-integration/src/connectors/archipel/transformers.rs(1 hunks)backend/connector-integration/src/types.rs(2 hunks)backend/domain_types/src/connector_types.rs(2 hunks)backend/domain_types/src/payment_address.rs(1 hunks)backend/domain_types/src/types.rs(2 hunks)backend/external-services/src/service.rs(2 hunks)backend/grpc-api-types/proto/payment.proto(2 hunks)backend/interfaces/src/connector_integration_v2.rs(2 hunks)config/development.toml(1 hunks)config/production.toml(1 hunks)config/sandbox.toml(1 hunks)examples/example-hs-grpc/proto/payment.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-13T07:57:48.659Z
Learnt from: iemyashasvi
Repo: juspay/connector-service PR: 194
File: config/development.toml:62-63
Timestamp: 2025-10-13T07:57:48.659Z
Learning: For Worldpay Vantiv development environment in config/development.toml, the correct URLs are:
- base_url: "https://transact.vantivprelive.com/vap/communicator/online"
- secondary_base_url: "https://onlinessr.vantivprelive.com"
Applied to files:
config/production.tomlconfig/development.tomlconfig/sandbox.toml
📚 Learning: 2025-10-13T08:02:24.195Z
Learnt from: iemyashasvi
Repo: juspay/connector-service PR: 194
File: config/sandbox.toml:54-55
Timestamp: 2025-10-13T08:02:24.195Z
Learning: For Worldpay Vantiv sandbox environment in config/sandbox.toml, the correct URLs are:
- base_url: "https://www.testvantivcnp.com/sandbox/new/sandbox/communicator/online"
- secondary_base_url: "https://www.testvantivcnp.com/sandbox"
Applied to files:
config/production.tomlconfig/development.tomlconfig/sandbox.toml
📚 Learning: 2025-09-26T08:08:52.624Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 193
File: backend/connector-integration/src/connectors/rapyd/transformers.rs:505-524
Timestamp: 2025-09-26T08:08:52.624Z
Learning: In Rapyd connector (backend/connector-integration/src/connectors/rapyd/transformers.rs), the next_action field in ResponseData struct is required (not Optional) as per Rapyd API documentation. This field always appears in payment responses with values like "3d_verification", "pending_capture", "pending_confirmation", or "not_applicable", consistent with hyperswitch connector patterns.
Applied to files:
backend/grpc-api-types/proto/payment.protobackend/domain_types/src/types.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-24T08:45:31.195Z
Learnt from: Anurag-05-prog
Repo: juspay/connector-service PR: 188
File: backend/connector-integration/src/connectors/peachpayments.rs:306-315
Timestamp: 2025-09-24T08:45:31.195Z
Learning: In Peachpayments connector (backend/connector-integration/src/connectors/peachpayments.rs), the connector_transaction_id field has different types across flows: PaymentVoidData uses String directly, while PaymentsCaptureData uses ResponseId enum that requires pattern matching or get_connector_transaction_id() method.
Applied to files:
backend/domain_types/src/connector_types.rsbackend/domain_types/src/types.rsbackend/connector-integration/src/types.rsbackend/connector-integration/src/connectors/archipel/transformers.rsbackend/connector-integration/src/connectors/archipel.rs
📚 Learning: 2025-09-15T06:56:22.535Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 172
File: backend/connector-integration/src/connectors/trustpay.rs:747-762
Timestamp: 2025-09-15T06:56:22.535Z
Learning: In Trustpay connector (backend/connector-integration/src/connectors/trustpay.rs), the refund_connector_metadata field has different data formats in different contexts: RSync expects it to be already parsed as JSON Value (accessing directly with .get()), while other refund flows expect it to be a JSON string that needs serde_json::from_str() parsing. The extract_payment_method_from_refund_metadata helper is designed for the latter case and should not be used in RSync.
Applied to files:
backend/domain_types/src/connector_types.rsbackend/domain_types/src/types.rsbackend/connector-integration/src/connectors/archipel/transformers.rsbackend/connector-integration/src/connectors/archipel.rs
📚 Learning: 2025-09-24T10:18:33.115Z
Learnt from: Anurag-05-prog
Repo: juspay/connector-service PR: 188
File: backend/connector-integration/src/connectors/peachpayments/transformers.rs:50-56
Timestamp: 2025-09-24T10:18:33.115Z
Learning: In Peach Payments connector API (backend/connector-integration/src/connectors/peachpayments/transformers.rs), the authorize request struct uses the field "charge_method" as expected by the Peach Payments API, not "paymentType" or other variations.
Applied to files:
backend/domain_types/src/types.rsbackend/connector-integration/src/connectors/archipel/transformers.rsbackend/connector-integration/src/connectors/archipel.rs
🧬 Code graph analysis (2)
backend/connector-integration/src/connectors/archipel/transformers.rs (2)
backend/connector-integration/src/connectors/macros.rs (1)
router_data(94-105)backend/domain_types/src/connector_types.rs (3)
from(2216-2412)to_connector_meta(681-688)std(686-686)
backend/connector-integration/src/connectors/archipel.rs (3)
backend/connector-integration/src/connectors/macros.rs (2)
router_data(94-105)request_body(66-75)backend/interfaces/src/connector_integration_v2.rs (5)
get_content_type(59-61)get_error_response_v2(136-145)get_headers(51-56)get_url(69-76)get_ca_certificate(213-221)backend/connector-integration/src/connectors/archipel/transformers.rs (16)
try_from(59-66)try_from(77-80)try_from(301-333)try_from(403-436)try_from(619-630)try_from(643-684)try_from(694-788)try_from(812-865)try_from(890-952)try_from(960-1009)try_from(1048-1064)try_from(1077-1118)try_from(1205-1222)try_from(1235-1256)try_from(1282-1303)try_from(1350-1443)
🔇 Additional comments (13)
backend/domain_types/src/payment_address.rs (1)
311-313: Newget_optional_zipaccessor is consistent and safe
get_optional_zipmirrors the existing optional accessors (get_optional_line2, names, etc.), correctly cloningOption<Secret<String>>and introducing no behavior change. Looks good as an additional helper.backend/external-services/src/service.rs (1)
665-681: Wiringca_certificateintocreate_clientlooks correct and side‑effect freePassing
request.ca_certificatethrough tocreate_clientcleanly extends the API without altering existing behavior when the field isNone. No issues from this call site as long as upstream code only populatesca_certificatewhen a valid PEM is available.config/production.toml (1)
57-57: LGTM! Configuration follows established patterns.The Archipel base URL configuration is correctly added and follows the same templated pattern used by other connectors like Adyen. The
{{merchant_endpoint_prefix}}placeholder enables environment-aware endpoint construction as described in the AI summary.backend/interfaces/src/connector_integration_v2.rs (2)
212-221: LGTM! CA certificate support properly integrated.The new
get_ca_certificatemethod follows the established pattern ofget_certificateandget_certificate_key, enabling one-way TLS verification. The default implementation safely returnsNone, allowing connectors to opt-in as needed.
108-108: LGTM! CA certificate correctly wired into request builder.The CA certificate is properly integrated into the request building flow, positioned after the mutual TLS certificate/key setup.
backend/connector-integration/src/connectors.rs (1)
92-93: LGTM! Standard connector module integration.The Archipel module is correctly declared and exported, following the established pattern used by all other connectors in this file.
backend/domain_types/src/connector_types.rs (2)
76-76: LGTM! Archipel correctly added to ConnectorEnum.The Archipel variant is properly added to the enum, maintaining consistency with the codebase's connector registry pattern.
115-115: LGTM! gRPC mapping correctly implemented.The mapping from the gRPC Connector enum to the internal ConnectorEnum is properly implemented for Archipel.
backend/connector-integration/src/types.rs (2)
7-7: LGTM! Import correctly added.The Archipel import is properly included alongside other connector imports.
63-63: LGTM! Connector instantiation correctly implemented.The Archipel connector is properly instantiated in the convert_connector match, following the established pattern of
Box::new(Archipel::new()).config/development.toml (1)
65-65: Archipel identical URLs are intentional and correctly configured.Verification confirms the identical Archipel base URLs across development, sandbox, and production are an intentional design choice. The configuration uses placeholder substitution (
{{merchant_endpoint_prefix}}), where the actual endpoint differentiation occurs at runtime when the merchant_endpoint_prefix value is injected per environment. This is a valid architectural pattern used similarly by other connectors (e.g., Adyen).backend/grpc-api-types/proto/payment.proto (1)
459-459: ARCHIPEL connector type added to enum.The new
ARCHIPEL = 95enum value is properly positioned and follows the sequential naming convention established in the codebase. This integrates with the broader Archipel connector integration outlined in the PR.backend/connector-integration/src/connectors/archipel.rs (1)
926-941: Addbuild_env_specific_endpointcall to RSync'sget_urlmethod to ensure merchant endpoint prefix substitution is applied consistently with other flowsThe verification confirms your concern. RSync (lines 926-941) uniquely bypasses
build_env_specific_endpoint, while all other flows—Refund, PSync, Capture, Void, and SetupMandate—use it to substitute the{{merchant_endpoint_prefix}}placeholder. Thebuild_env_specific_endpointfunction explicitly replaces this placeholder with the actual endpoint prefix from connector metadata.RSync should call
build_env_specific_endpointwithreq.request.connector_metadata(following the same pattern as the Refund flow at line 884-885), or the base URL must be guaranteed to be fully expanded without placeholders in configuration. Without this fix, refund sync will fail in multi-tenant deployments requiring merchant-specific endpoint prefixes.⛔ Skipped due to learnings
Learnt from: Shubhodip900 Repo: juspay/connector-service PR: 172 File: backend/connector-integration/src/connectors/trustpay.rs:747-762 Timestamp: 2025-09-15T06:56:22.535Z Learning: In Trustpay connector (backend/connector-integration/src/connectors/trustpay.rs), the refund_connector_metadata field has different data formats in different contexts: RSync expects it to be already parsed as JSON Value (accessing directly with .get()), while other refund flows expect it to be a JSON string that needs serde_json::from_str() parsing. The extract_payment_method_from_refund_metadata helper is designed for the latter case and should not be used in RSync.Learnt from: Shubhodip900 Repo: juspay/connector-service PR: 95 File: backend/connector-integration/src/connectors/noon.rs:0-0 Timestamp: 2025-08-06T08:45:01.878Z Learning: In the Noon connector, connector_refund_id is populated from API responses and is already validated upstream. The field cannot be empty due to struct constraints, and special URL characters like '/', '?', '#', '&' will never appear since the IDs come from Noon's API responses which follow their alphanumeric format.
| ACI = 1; | ||
| ADYEN = 2; | ||
| AIRWALLEX = 3; | ||
| ARCHIPEL = 90; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify enum value consistency across proto files.
There's a discrepancy: this file uses ARCHIPEL = 90, but according to the AI summary, backend/grpc-api-types/proto/payment.proto uses ARCHIPEL = 95. Proto enum values should be consistent across all proto files to avoid serialization issues.
Additionally, a past review comment on this line stated "this is not needed" – please clarify whether this example proto should include the ARCHIPEL enum value.
🏁 Script executed:
#!/bin/bash
# Verify ARCHIPEL enum values across all proto files
rg -n "ARCHIPEL\s*=\s*\d+" --type protoLength of output: 29
🏁 Script executed:
#!/bin/bash
# Find all proto files and search for ARCHIPEL enum values
echo "=== Finding all .proto files ==="
fd -e proto
echo -e "\n=== Searching for ARCHIPEL in all proto files ==="
rg -n "ARCHIPEL" -g "*.proto"
echo -e "\n=== Checking payment.proto files specifically ==="
find . -name "payment.proto" -type f | head -20Length of output: 587
🏁 Script executed:
#!/bin/bash
# Check context around ARCHIPEL in both files
echo "=== Context around ARCHIPEL in examples/example-hs-grpc/proto/payment.proto (lines 110-130) ==="
sed -n '110,130p' examples/example-hs-grpc/proto/payment.proto
echo -e "\n=== Context around ARCHIPEL in backend/grpc-api-types/proto/payment.proto (lines 450-470) ==="
sed -n '450,470p' backend/grpc-api-types/proto/payment.proto
echo -e "\n=== Check if these are in the same enum and its name ==="
echo "Example file enum:"
sed -n '100,130p' examples/example-hs-grpc/proto/payment.proto | grep -A5 -B5 "enum"
echo -e "\nBackend file enum:"
sed -n '440,470p' backend/grpc-api-types/proto/payment.proto | grep -A5 -B5 "enum"Length of output: 1367
Fix enum value mismatch for ARCHIPEL in Connector enum.
The ARCHIPEL enum value is inconsistent across proto files:
examples/example-hs-grpc/proto/payment.proto:121usesARCHIPEL = 90backend/grpc-api-types/proto/payment.proto:459usesARCHIPEL = 95
Both define the same Connector enum. Mismatched enum values will cause serialization incompatibility and prevent gRPC interoperability between the example code and the actual backend. Update the example proto file to use ARCHIPEL = 95 to match the backend definition.
🤖 Prompt for AI Agents
In examples/example-hs-grpc/proto/payment.proto around line 121, the Connector
enum value ARCHIPEL is set to 90 but the backend definition uses 95; change the
enum value for ARCHIPEL to 95 in this example proto so it matches
backend/grpc-api-types/proto/payment.proto and ensure both proto files use
identical numeric values for the Connector enum to avoid serialization
incompatibility.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/connector-integration/src/connectors.rs(1 hunks)backend/connector-integration/src/connectors/archipel.rs(1 hunks)backend/connector-integration/src/connectors/archipel/transformers.rs(1 hunks)backend/connector-integration/src/types.rs(2 hunks)backend/domain_types/src/types.rs(2 hunks)backend/external-services/src/service.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/connector-integration/src/connectors.rs
- backend/domain_types/src/types.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-24T08:45:31.195Z
Learnt from: Anurag-05-prog
Repo: juspay/connector-service PR: 188
File: backend/connector-integration/src/connectors/peachpayments.rs:306-315
Timestamp: 2025-09-24T08:45:31.195Z
Learning: In Peachpayments connector (backend/connector-integration/src/connectors/peachpayments.rs), the connector_transaction_id field has different types across flows: PaymentVoidData uses String directly, while PaymentsCaptureData uses ResponseId enum that requires pattern matching or get_connector_transaction_id() method.
Applied to files:
backend/connector-integration/src/types.rsbackend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-15T04:33:18.323Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 172
File: backend/grpc-server/tests/trustpay_payment_flows_test.rs:1-4
Timestamp: 2025-09-15T04:33:18.323Z
Learning: In test files for the juspay/connector-service repository, broad Clippy suppressions (#![allow(clippy::expect_used)], #![allow(clippy::unwrap_used)], #![allow(clippy::panic)]) are used as an established coding convention, as confirmed by the maintainer.
Applied to files:
backend/connector-integration/src/types.rs
📚 Learning: 2025-09-15T06:56:22.535Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 172
File: backend/connector-integration/src/connectors/trustpay.rs:747-762
Timestamp: 2025-09-15T06:56:22.535Z
Learning: In Trustpay connector (backend/connector-integration/src/connectors/trustpay.rs), the refund_connector_metadata field has different data formats in different contexts: RSync expects it to be already parsed as JSON Value (accessing directly with .get()), while other refund flows expect it to be a JSON string that needs serde_json::from_str() parsing. The extract_payment_method_from_refund_metadata helper is designed for the latter case and should not be used in RSync.
Applied to files:
backend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-24T10:18:33.115Z
Learnt from: Anurag-05-prog
Repo: juspay/connector-service PR: 188
File: backend/connector-integration/src/connectors/peachpayments/transformers.rs:50-56
Timestamp: 2025-09-24T10:18:33.115Z
Learning: In Peach Payments connector API (backend/connector-integration/src/connectors/peachpayments/transformers.rs), the authorize request struct uses the field "charge_method" as expected by the Peach Payments API, not "paymentType" or other variations.
Applied to files:
backend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-26T08:08:52.624Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 193
File: backend/connector-integration/src/connectors/rapyd/transformers.rs:505-524
Timestamp: 2025-09-26T08:08:52.624Z
Learning: In Rapyd connector (backend/connector-integration/src/connectors/rapyd/transformers.rs), the next_action field in ResponseData struct is required (not Optional) as per Rapyd API documentation. This field always appears in payment responses with values like "3d_verification", "pending_capture", "pending_confirmation", or "not_applicable", consistent with hyperswitch connector patterns.
Applied to files:
backend/connector-integration/src/connectors/archipel/transformers.rs
🧬 Code graph analysis (2)
backend/connector-integration/src/connectors/archipel.rs (3)
backend/connector-integration/src/connectors/macros.rs (2)
router_data(94-105)request_body(66-75)backend/interfaces/src/connector_integration_v2.rs (4)
get_error_response_v2(136-145)get_headers(51-56)get_url(69-76)get_ca_certificate(213-221)backend/connector-integration/src/connectors/archipel/transformers.rs (17)
try_from(49-56)try_from(67-70)try_from(297-330)try_from(400-433)try_from(631-642)try_from(655-696)try_from(708-806)try_from(830-886)try_from(911-973)try_from(981-1028)try_from(1067-1086)try_from(1099-1140)try_from(1256-1272)try_from(1285-1306)try_from(1332-1355)try_from(1402-1495)serde_json(520-520)
backend/connector-integration/src/connectors/archipel/transformers.rs (2)
backend/connector-integration/src/connectors/macros.rs (1)
router_data(94-105)backend/domain_types/src/connector_types.rs (3)
from(2216-2412)to_connector_meta(681-688)std(686-686)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
backend/connector-integration/src/connectors/archipel/transformers.rs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| fn to_connector_meta( |
There was a problem hiding this comment.
This can be an util function, also check whether similar util function exists or not.
backend/connector-integration/src/connectors/archipel/transformers.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/connector-integration/src/connectors/archipel.rs (1)
680-697: Remove leftoverprintln!from authorize URL builder.The
println!("here");in theAuthorizeget_urlis a stray debug statement and will spam stdout in production. Please remove it or replace it with a propertracing::debug!call if you really need this signal.
🧹 Nitpick comments (5)
backend/external-services/src/service.rs (1)
734-815: Previous critical mutual TLS issue has been resolved.The rewritten match-based implementation now correctly handles mutual TLS by creating a
reqwest::Identityfrom the client certificate and key, then building a client with that identity installed (lines 747-753). This addresses the previous critical regression where the client identity was never loaded.The four-case structure is clear and handles the TLS configuration matrix appropriately:
- Mutual TLS with optional CA verification
- One-way TLS with CA verification only
- Invalid partial configuration
- Default no-certificate client
However, consider the following improvements:
1. Line ending normalization is incomplete (lines 740-741, 758, 782)
The
.replace("\\r\\n", "\n")only handles escaped newlines (literal backslash-r-backslash-n sequences, as might appear in JSON). If the PEM string contains actual CRLF line endings (\r\n), they won't be normalized. Depending on how certificates are sourced, this could cause parsing failures.Consider handling both escaped and actual line endings:
-let cert_pem = client_cert.expose().replace("\\r\\n", "\n"); -let key_pem = client_key.expose().replace("\\r\\n", "\n"); +let cert_pem = client_cert.expose() + .replace("\\r\\n", "\n") // Handle escaped newlines + .replace("\r\n", "\n"); // Handle actual CRLF +let key_pem = client_key.expose() + .replace("\\r\\n", "\n") + .replace("\r\n", "\n");Apply similar changes to lines 758 and 782.
2. Invalid configuration silently falls back to base client (lines 804-807)
When only one of cert or key is provided, the code logs a warning and falls back to
get_base_client. This silent degradation might mask configuration errors. Consider whether this should return an error instead, forcing the caller to provide valid configuration.If you want to enforce strict validation:
(Some(_), None, _) | (None, Some(_), _) => { - tracing::warn!("Incomplete client certificate configuration: both cert and key are required for mutual TLS"); - get_base_client(proxy_config, should_bypass_proxy) + tracing::error!("Incomplete client certificate configuration: both cert and key are required for mutual TLS"); + Err(report!(ApiClientError::InvalidProxyConfiguration)) + .attach_printable("Mutual TLS requires both client certificate and private key") }3. Document why rustls is required (lines 753, 789)
The mutual TLS and one-way TLS paths explicitly call
.use_rustls_tls(), but the default base client does not. Consider adding a comment explaining why rustls is required for custom certificate handling (e.g., better PEM support, consistent behavior across platforms).backend/connector-integration/src/connectors/archipel.rs (1)
733-749: Fix PSync error message string to reference the correct flow.The attached printable message uses
"connector_meta is required for Archipel capture flow"inside the PSyncget_urlpath, which is misleading when debugging PSync issues. Consider updating the string to say"PSync flow"(or similar) to match the actual context.backend/connector-integration/src/connectors/archipel/transformers.rs (3)
739-793: Use logical||instead of bitwise|for boolean combinations.
is_saved_card_paymentandstored_on_fileuse|on booleans. This works, but||is the idiomatic operator and signals intent more clearly, especially to future readers.Example:
- let is_saved_card_payment = (router_data.request.is_mandate_payment()) - | (router_data.request.setup_future_usage == Some(FutureUsage::OnSession)); + let is_saved_card_payment = router_data.request.is_mandate_payment() + || router_data.request.setup_future_usage == Some(FutureUsage::OnSession); - let stored_on_file = - is_saved_card_payment | router_data.request.is_customer_initiated_mandate_payment(); + let stored_on_file = + is_saved_card_payment || router_data.request.is_customer_initiated_mandate_payment();
880-921: Factor out shared 3DS-from-metadata parsing to avoid duplication.The logic that pulls
authentication_data(CAVV, ECI,ds_trans_id,message_version, timestamp) fromrequest.metadatato buildArchipel3DSis duplicated here and again in the SetupMandate transformer. Extracting this into a small helper (e.g.,fn three_ds_from_metadata(metadata: &Value) -> Option<Archipel3DS>) would reduce drift if the required fields or mapping change.This is a maintainability improvement only; current behavior is consistent and forwards 3DS data instead of dropping it.
1190-1239: Validate error-to-attempt-status mapping against Archipel’s error codes.
get_error_attempt_statuscurrently infersAttemptStatusfrom simple substring checks onerror_codeplus the HTTP status. That’s reasonable as a first pass, but it’s heuristic; you might want to double‑check against Archipel’s documented error codes to ensure:
- Card declines are always mapped to
Failure(or whatever is expected by upstream),- Auth failures are consistently mapped to
AuthenticationFailed,- Rate‑limit / server issues (
429,5xx) being treated asPendingmatches your retry strategy.If the mapping is correct, consider adding a short comment referencing the spec section or an internal doc so it’s clear this logic is intentional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/connector-integration/src/connectors/archipel.rs(1 hunks)backend/connector-integration/src/connectors/archipel/transformers.rs(1 hunks)backend/external-services/src/service.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-24T08:45:31.195Z
Learnt from: Anurag-05-prog
Repo: juspay/connector-service PR: 188
File: backend/connector-integration/src/connectors/peachpayments.rs:306-315
Timestamp: 2025-09-24T08:45:31.195Z
Learning: In Peachpayments connector (backend/connector-integration/src/connectors/peachpayments.rs), the connector_transaction_id field has different types across flows: PaymentVoidData uses String directly, while PaymentsCaptureData uses ResponseId enum that requires pattern matching or get_connector_transaction_id() method.
Applied to files:
backend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-15T06:56:22.535Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 172
File: backend/connector-integration/src/connectors/trustpay.rs:747-762
Timestamp: 2025-09-15T06:56:22.535Z
Learning: In Trustpay connector (backend/connector-integration/src/connectors/trustpay.rs), the refund_connector_metadata field has different data formats in different contexts: RSync expects it to be already parsed as JSON Value (accessing directly with .get()), while other refund flows expect it to be a JSON string that needs serde_json::from_str() parsing. The extract_payment_method_from_refund_metadata helper is designed for the latter case and should not be used in RSync.
Applied to files:
backend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-15T04:33:18.323Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 172
File: backend/grpc-server/tests/trustpay_payment_flows_test.rs:1-4
Timestamp: 2025-09-15T04:33:18.323Z
Learning: In test files for the juspay/connector-service repository, broad Clippy suppressions (#![allow(clippy::expect_used)], #![allow(clippy::unwrap_used)], #![allow(clippy::panic)]) are used as an established coding convention, as confirmed by the maintainer.
Applied to files:
backend/connector-integration/src/connectors/archipel.rs
📚 Learning: 2025-09-24T10:18:33.115Z
Learnt from: Anurag-05-prog
Repo: juspay/connector-service PR: 188
File: backend/connector-integration/src/connectors/peachpayments/transformers.rs:50-56
Timestamp: 2025-09-24T10:18:33.115Z
Learning: In Peach Payments connector API (backend/connector-integration/src/connectors/peachpayments/transformers.rs), the authorize request struct uses the field "charge_method" as expected by the Peach Payments API, not "paymentType" or other variations.
Applied to files:
backend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
📚 Learning: 2025-09-26T08:08:52.624Z
Learnt from: Shubhodip900
Repo: juspay/connector-service PR: 193
File: backend/connector-integration/src/connectors/rapyd/transformers.rs:505-524
Timestamp: 2025-09-26T08:08:52.624Z
Learning: In Rapyd connector (backend/connector-integration/src/connectors/rapyd/transformers.rs), the next_action field in ResponseData struct is required (not Optional) as per Rapyd API documentation. This field always appears in payment responses with values like "3d_verification", "pending_capture", "pending_confirmation", or "not_applicable", consistent with hyperswitch connector patterns.
Applied to files:
backend/connector-integration/src/connectors/archipel.rsbackend/connector-integration/src/connectors/archipel/transformers.rs
🧬 Code graph analysis (2)
backend/connector-integration/src/connectors/archipel.rs (1)
backend/connector-integration/src/connectors/archipel/transformers.rs (18)
try_from(49-56)try_from(67-70)try_from(297-330)try_from(400-433)try_from(637-648)try_from(661-702)try_from(714-812)try_from(836-933)try_from(958-1020)try_from(1028-1075)try_from(1114-1133)try_from(1146-1187)try_from(1303-1319)try_from(1332-1353)try_from(1379-1402)try_from(1449-1583)serde_json(520-520)from_response(516-534)
backend/connector-integration/src/connectors/archipel/transformers.rs (2)
backend/domain_types/src/types.rs (1)
RawCardNumber(796-796)backend/common_utils/src/lib.rs (1)
date_as_yyyymmddthhmmssmmmz(120-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
backend/external-services/src/service.rs (2)
680-680: CA certificate parameter added correctly.The
ca_certificateparameter is now passed through from the request tocreate_client, enabling server certificate verification in mutual TLS and one-way TLS scenarios.
730-733: Function signature updated appropriately.The addition of the
ca_certificateparameter completes the TLS configuration surface, allowing callers to specify server CA certificates for verification in addition to client certificates for mutual TLS.backend/connector-integration/src/connectors/archipel/transformers.rs (1)
1309-1319: Confirm connector metadata shape for refund tenant resolution.
ArchipelRefundRequest::try_fromexpectsrequest.connector_metadatato contain Archipel config (viaArchipelConfigData::try_from→to_connector_meta), while other flows usemetadataor nested"connector_meta_data"values. Please ensure that the producer for refund requests is indeed sending connector metadata in the expected format (either direct JSON string or object with"connector_meta_data"), otherwise tenant resolution will fail at runtime.
Description
Motivation and Context
Additional Changes
How did you test it?