Skip to content

fix(v4)!: restore --currency for live finance methods (regression from #441/#442/#443)#450

Merged
axisrow merged 1 commit into
mainfrom
fix/v4-live-currency-regression
May 29, 2026
Merged

fix(v4)!: restore --currency for live finance methods (regression from #441/#442/#443)#450
axisrow merged 1 commit into
mainfrom
fix/v4-live-currency-regression

Conversation

@axisrow

@axisrow axisrow commented May 28, 2026

Copy link
Copy Markdown
Owner

Summary

Root cause (verified, reproducible)

curl -sL https://yandex.ru/dev/direct/doc/dg-v4/live/PayCampaigns.html \\
  | grep -oE 'PayCampElement[^}]+\\}' | head -1
# → PayCampElement */ "CampaignID" : (int), "Sum" : (float), "Currency" : (string) }

Live pages for all three methods (TransferMoney / PayCampaigns / CreateInvoice) contain:

  1. JSON request schema with Currency: (string) inside PayCampElement.
  2. Parameter table row Currency → required: Да.
  3. Changelog «Новое в версии Live 4: Входной параметр Currency стал обязательным».
  4. Request examples with 'Currency': 'RUB'.

Reference URLs lack all four — they describe v4 before Live 4.

Full root-cause diff and per-URL comparison table is in the issue audit comment: #125 (comment)

Test plan

  • pytest tests/ — 1908 passed, 84 skipped (existing skips: opt-in live writes).
  • direct v4finance transfer-money --from-campaign-id 1 --to-campaign-id 2 --amount 100.50 --currency RUB --finance-token X --operation-num 1 --dry-run — JSON contains Currency: "RUB" in both items.
  • direct v4finance pay-campaigns --pay-method Overdraft ... accepted (without --contract-id).
  • direct v4finance create-invoice --payment 1=10 --currency RUB ... emits Currency in each Payments[] item.
  • Opt-in live verification under YANDEX_DIRECT_LIVE_FINANCE_WRITE=1 once a sandbox campaign is available (test body already updated to include Currency: "RUB").

Audit comment posted to #125

#125 (comment)

…#441/#442/#443)

PRs #441/#442/#443 dropped the obligatory Live 4 input parameter
Currency from PayCampElement on transfer-money, pay-campaigns, and
create-invoice; #442 additionally dropped the valid Overdraft
PayMethod. The commits cited dg-v4/reference/<Method> as the
verification source. That URL is legacy v4 (pre-Live), where
PayCampElement is {CampaignID, Sum} only. The CLI ships these commands
in the v4finance Live group and must mirror dg-v4/live/<Method>.

Verified 2026-05-29 by curling the live pages:
- dg-v4/live/TransferMoney → PayCampElement = {CampaignID, Sum, Currency};
  changelog "Новое в версии Live 4: Входной параметр Currency стал
  обязательным".
- dg-v4/live/PayCampaigns → PayCampElement = {CampaignID, Sum, Currency};
  same changelog plus "Добавлен метод оплаты Overdraft".
- dg-v4/live/CreateInvoice → PayCampElement = {CampaignID, Sum, Currency}.

Restored:
- --currency (required, Choice: RUB/CHF/EUR/KZT/TRY/UAH/USD/BYN) on
  transfer-money, pay-campaigns, create-invoice.
- Currency emitted on every Payments[] / FromCampaigns[] / ToCampaigns[]
  item.
- Overdraft re-enabled in V4_PAY_METHODS; --contract-id remains required
  only when --pay-method Bank.

Updated v4_contracts.py example_param and notes (now pointing at
dg-v4/live/*); updated tests/test_v4finance_money.py and
tests/test_v4_contracts.py expectations; updated README, CHANGELOG
(0.3.16 BREAKING CHANGES), scripts/test_dangerous_commands.sh checklist,
tests/API_COVERAGE.md, and the opt-in live-write test for
CreateInvoice. Bumped version to 0.3.16.

Audit trail: #125 (comment)

Co-Authored-By: Claude Opus 4.7 <[email protected]>
@axisrow

axisrow commented May 29, 2026

Copy link
Copy Markdown
Owner Author

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

@claude

claude Bot commented May 29, 2026

Copy link
Copy Markdown

Claude finished @axisrow's task in 1m 16s —— View job


Review: PR #450 — restore --currency for v4 live finance methods

  • Read changed files (v4finance.py, v4_contracts.py, tests)
  • Verify the regression-fix logic and wire-field handling
  • Check Overdraft pay-method re-enablement
  • Review tests for correctness
  • Post review findings

Verdict: no critical issues. Safe to merge. The change is correct, well-scoped, and the breaking nature is properly handled.

What I verified

1. Wire-field restoration is consistent across all three methods. Currency is added to every payment item:

  • transfer_money — both FromCampaigns[] and ToCampaigns[] (v4finance.py:500-516)
  • pay_campaigns — each Payments[] element (v4finance.py:606-613)
  • create_invoice via _invoice_payments_param(payments, currency) (v4finance.py:62-69)

The wire shape now matches the dg-v4/live/* JSON schema the PR cites. Root-cause framing (reference-docs vs Live-docs) is sound and the curl evidence is reproducible.

2. pay-campaigns Overdraft logic is correct — no data loss. v4finance.py:602-617:

contract_id = (contract_id or "").strip()
if pay_method == "Bank" and not contract_id:
    raise click.UsageError("--contract-id is required when --pay-method Bank")
...
if contract_id:
    param["ContractID"] = contract_id

Bank still requires --contract-id; Overdraft correctly omits ContractID from the payload. The new test (test_pay_campaigns_accepts_overdraft_pay_method_for_direct_advertisers) asserts "ContractID" not in body["param"] — good coverage of the branch.

3. Test coverage is thorough. Every invocation path that now requires --currency was updated (dry-run, env-fallback, master-token derivation, credential-conflict, validation-before-API, mocked-response), and the live contract write body (test_v4_live_contracts.py:202) includes Currency. The renamed test_pay_campaigns_rejects_undocumented_pay_method now uses CreditCard as the rejected value, keeping the negative case meaningful after Overdraft became valid.

Non-blocking observations (not requesting changes)

  • Breaking change is intentional and documented. --currency is now required=True, so any existing automation calling these three commands without it will fail at parse time. This is the correct behavior per the Live 4 spec (Currency obligatory), the CHANGELOG has a BREAKING CHANGES section, and the version is bumped to 0.3.16. Flagging only so it's a conscious decision — consumers must update their scripts.
  • currency.upper() is redundant given click.Choice(..., case_sensitive=False) already returns the canonical (uppercase) choice value. Harmless no-op, not worth changing.

No security, performance, or logical-error concerns found.
· branch fix/v4-live-currency-regression

@axisrow

axisrow commented May 29, 2026

Copy link
Copy Markdown
Owner Author

Триаж ревью: блокирующих (FIX) замечаний нет, CI зелёный — мержу.

Два non-blocking замечания ревьюера (оба SKIP, изменений не требуют):

  • Breaking change (--currency теперь required) — осознанное решение, соответствует спецификации Live 4 (Currency обязателен), задокументировано в CHANGELOG (BREAKING CHANGES) и версия поднята до 0.3.16. Оставляем как есть.
  • currency.upper() избыточен при case_sensitive=False — косметика, harmless no-op; на функциональность не влияет, не трогаем.

@axisrow
axisrow merged commit d35208d into main May 29, 2026
6 checks passed
@axisrow
axisrow deleted the fix/v4-live-currency-regression branch May 29, 2026 03:24
axisrow added a commit that referenced this pull request May 29, 2026
* chore(audit): add docs/wire-shape scanner + 2026-05-29 sweep (refs #111)

New offline-rerunnable tool that parses Yandex Direct docs HTML, extracts the
request JSON-schema, and compares with V4_METHOD_CONTRACTS.example_param and
RESOURCE_MAPPING_V5. Closes the methodological gap that allowed the v4
Currency regression (root-caused in #125, fixed by #450) — that PR happened
because verification was done against dg-v4/reference/ pages instead of
dg-v4/live/ ones.

Tooling
-------
- scripts/audit_wire_shape.py — modes: --v4, --v5, --reports, --all.
  Uses a 3-retry × 5-second-pause captcha guard reusing the same markers
  as direct_cli.reports_coverage.fetch_reports_spec. Persistent captcha is
  recorded as `captcha-after-retries` rather than aborting the run.
- tests/test_audit_wire_shape.py — 9 offline golden tests covering
  request-section extraction, schema parsing, response-side leak
  prevention, Live 4 hint detection, example_param key walk, captcha
  marker detection, and v5 per-method URL derivation.

Sweep artefacts (2026-05-29)
----------------------------
- docs/audits/PROJECT_WIRE_SHAPE_AUDIT_2026-05-29.md — 174 findings
  across 32 v4 methods, ~112 v5 operations, 1 reports surface.
- docs/audits/wire_shape.json — same data, machine-readable.

Reproducibility
---------------
    python scripts/audit_wire_shape.py --all \
        --json docs/audits/wire_shape.json \
        --markdown docs/audits/PROJECT_WIRE_SHAPE_AUDIT_2026-05-29.md

Summary writeup with severity-classified findings posted as a comment to
issue #111 (link in the comment body).

This PR adds the auditing capability and the dated report only. Individual
contract / payload-builder fixes for the HIGH-severity gaps (GetEventsLog
contract missing 11 fields, CreateNewForecast missing 3+ fields,
GetBannersTags missing CampaignIDS) are deliberately deferred to follow-up
PRs — one per gap, per the project's "1 issue = 1 PR" rule.

Audit trail: #111 (comment)

Co-Authored-By: Claude Opus 4.7 <[email protected]>

* fix(audit): preserve resource name in v5 per-method URLs + dedup spec URL

Review fixes for #451:

- _v5_per_method_url chopped the trailing path segment unconditionally,
  so single-segment docs bases (vcards, smartadtargets, dynamictextadtargets,
  dynamicfeedadtargets) lost their resource name and resolved to
  `…/ru/get` — silently mis-auditing 4 v5 groups at 404/captcha URLs.
  Now the tail is chopped only when it duplicates its parent segment;
  single-segment bases append the method to the full base.
- The test that locked in the buggy `…/ru/get` output is corrected and a
  vcards regression-guard case is added.
- REPORTS_SPEC_URL now imports REPORTS_SPEC_URLS["spec"] from the registry
  instead of duplicating the literal, per CLAUDE.md "No URL literals
  outside the registry".

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* fix(audit): gate notes-points-at-reference on a verified live page

The FINDING_NOTES_REFERENCE_URL check fired whenever the contract notes
cited dg-v4/reference/ and url_used stayed live_url — but a captcha'd live
page also leaves url_used == live_url, so the finding asserted "live exists,
drop reference" on a page that was never actually read (15/16 v4 methods
were captcha-blocked in the 2026-05-29 run). Require fetched.status == "ok"
so the finding only fires when the live schema was genuinely verified.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

---------

Co-authored-by: hapi <[email protected]>
Co-authored-by: Claude Opus 4.7 <[email protected]>
Co-authored-by: axisrow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants