Prepare middleware for PolySafe de-whitelisting on Polygon

[valory-coding-agent]

Implements: https://linear.app/valory/issue/OPE-1917

Summary
Prepares the middleware for the upcoming PolySafe de-whitelisting on Polygon. Removes polymarket_trader from POLY_SAFE_SERVICE_NAMES so new Polystrat service deployments use the standard safe_multisig_with_recovery_module factory instead of the soon-to-be-de-whitelisted PolySafe factory. Adds UnauthorizedMultisigException with structured error handling so that if the multisig creator contract is already de-whitelisted, the user gets a clear HTTP 400 with error code UNAUTHORIZED_MULTISIG instead of a generic 500.

Technical Scope

Task A – Remove polymarket_trader from POLY_SAFE_SERVICE_NAMES

File: operate/constants.py

Change POLY_SAFE_SERVICE_NAMES from frozenset(("polymarket_trader",)) to frozenset().

Effect: new Polystrat service deployments will go through safe_multisig_with_recovery_module (the standard path) instead of the soon-to-be-de-whitelisted PolySafe factory (0xA749f605D93B3efcc207C54270d83C6E8fa70fF8). Existing services with an already-deployed Safe are unaffected.

Task B – Add UnauthorizedMultisigException and surface it as an actionable error

B-1 Define the exception

File: operate/exceptions.py

Add a new exception class UnauthorizedMultisigException (modelled on InsufficientFundsException):

• Accepts a human-readable msg string.
• Exposes a to_error_fields() method that returns {"error_code": "UNAUTHORIZED_MULTISIG"}.

B-2 Raise the exception in the mega-batch revert-attribution block

File: operate/services/manage.py – method _deploy_service_onchain_from_safe

In the except block that handles a reverted mega-batch (around L985-1015 where simulate_safe_sub_tx is called per sub-transaction):

1. Collect the error strings returned by each simulate_safe_sub_tx call.
2. After the attribution loop, if any collected error string contains the substring "UnauthorizedMultisig", raise UnauthorizedMultisigException with a message like: "Service deployment failed: the multisig creator contract is not whitelisted on-chain. Contact support."
3. If the substring is not found, re-raise the original settle_error (current behaviour, which ends up as a generic 500).

B-3 Map the exception to an HTTP 400 in the deploy endpoint

File: operate/cli.py – endpoint _deploy_and_run_service

Add a new except UnauthorizedMultisigException as e handler (between the existing InsufficientFundsException handler and the generic Exception handler):

• Return a JSON response with status 400 containing {"error": str(e), **e.to_error_fields()}.

B-4 Unit tests

• Test that POLY_SAFE_SERVICE_NAMES is now empty.
• Test UnauthorizedMultisigException.__str__ and .to_error_fields().
• Test the CLI endpoint: mock deploy_service_onchain_from_safe to raise UnauthorizedMultisigException, assert 400 response with the expected error_code.

[valory-coding-agent]

Code Review — OPE-1917

Scope: Prepare middleware for PolySafe de-whitelisting on Polygon
Files reviewed: 6 of 6
Comments: 1 ([security]: 0, [bug]: 0, [scope]: 1, [convention]: 0, [suggestion]: 0, [question]: 0, [human-input]: 0, [nit]: 0)

Key findings

• Missing unit test for the revert-attribution detection logic in manage.py — the scope explicitly requires a test that mocks simulate_safe_sub_tx to return "UnauthorizedMultisig" and asserts the named exception is raised. This is the most critical integration point in the PR and is currently untested.

Scope coverage

• ✅ operate/constants.py — modified (POLY_SAFE_SERVICE_NAMES set to empty frozenset)
• ✅ operate/exceptions.py — modified (UnauthorizedMultisigException added, follows InsufficientFundsException pattern)
• ✅ operate/services/manage.py — modified (revert attribution extended to detect UnauthorizedMultisig and raise named exception)
• ✅ operate/cli.py — modified (new except UnauthorizedMultisigException handler returning HTTP 400 with structured error)
• ✅ tests/test_manage_unit.py — constant assertion and exception class tests present
• ✅ tests/test_cli_unit.py — CLI endpoint handler test present
• ⚠️ tests/test_manage_unit.py — manage.py revert-attribution detection logic test missing (Section 4 / Section 9)

Not reviewed

• None — all changed files were reviewed

---

🤖 Coding Agent [Beta] · Automated review against OPE-1917

[OjusWiZard]

address the unresolved comments

[valory-coding-agent]

Addressed 2 review comment(s) ([scope]: 1, CodeQL security: 1).
Replied with justification to 0 skipped comment(s).
Left 0 [human-input] comment(s) for human action.
Re-triggering review.

[OjusWiZard]

Task A (constants.py) is correct and independently valuable — please keep it. Task B is the problem.

1. Drop Task B entirely (blocking)

We don't translate any other contract revert into a structured HTTP error — WrongServiceState, OnlyOwnServiceMultisig, ServiceMustBeInactive, staking-slot reverts and the rest all fall through to the generic 500. Special-casing one custom error out of that set is inconsistent, and it puts a substring match on a revert string into the deploy control flow, which is a fragile thing to maintain.

That is the main reason to drop it. But it also doesn't work, which is worth spelling out because it explains why CI is green:

UnauthorizedMultisig(address) is a Solidity custom error (defined on ServiceRegistryL2 and StakingToken), not a require string. simulate_safe_sub_tx does a raw eth_call with a plain dict and returns str(ContractLogicError); with no ABI in scope, web3 raises ContractCustomError(data, data=data) where the message is the raw ABI payload, never the name (web3/_utils/error_formatters_utils.py:126-133).

Reproduced against a realistic Geth revert response:

type:   ContractCustomError
str(e): ('0x14460f20000000000000000000000000a749f605...', '0x14460f20...')

"UnauthorizedMultisig" in str(e) -> False
"0x14460f20"           in str(e) -> True     # keccak("UnauthorizedMultisig(address)")[:4]

So the any(...) at manage.py:1009 is always False, control falls to the bare raise at manage.py:1015, and the user gets exactly the generic 500 this PR set out to replace. exceptions.py, the cli.py handler and both new tests are dead weight around a branch that never executes.

Please revert B-1 through B-4 and ship Task A on its own. If the goal is to give users an actionable message for de-whitelisting, that belongs in a separate PR with a design that generalises — decoding revert data against the known contract ABIs once, in simulate_safe_sub_tx or nearby, so every custom error benefits rather than this one.

2. If you'd rather keep it, these all need fixing first

• Match the selector, not the name — compare against 0x14460f20 / e.data, not the substring.
• Verify the sub-tx simulation even reaches the whitelist check. Attribution simulates each sub-tx independently against live pre-batch state. The deploy sub-tx needs FINISHED_REGISTRATION, which register_instances only establishes later in the same batch — so depending on ServiceRegistryL2.deploy's internal check ordering, the simulation may revert on the state precondition and never reach the multisig whitelist check at all. Needs confirming on a Tenderly fork before the match can be trusted.
• manage.py:1176-1179 — the stepwise resume path has no equivalent handling. The mega-batch path is entered only from PRE_REGISTRATION or in-batch teardown (manage.py:793); a service interrupted after register but before deploy resumes stepwise, where tx.settle() has no try/except at all. That's precisely the de-whitelisting scenario this PR targets, and it still returns a 500.
• manage.py:989-1007 — the broad except wraps the whole loop, not each iteration. An RPC error on an early sub-tx aborts attribution, so the reverting sub-tx is never simulated and errors never gets the marker. This was diagnostics-only before; the PR promotes errors to control flow, so incompleteness now silently changes the HTTP status. It also inverts simulate_safe_sub_tx's documented contract that transport errors "propagate, so callers fail loudly".

3. The new revert-attribution test confirms the bug rather than catching it

TestRevertAttributionUnauthorizedMultisig (added in d72d092) is the right shape — it finally reaches manage.py instead of injecting the exception downstream — but it is built on a fabricated fixture:

patch("operate.services.manage.simulate_safe_sub_tx",
      return_value="UnauthorizedMultisig(0xABC)")

Real simulate_safe_sub_tx never returns that. It returns str(ContractCustomError), which is the raw hex payload shown above. The mock hard-codes the very assumption that is wrong, so the test passes green while production silently takes the raise branch. A test whose fixture is invented rather than captured from the real call gives false confidence — if this handling is kept, pin the fixture to an actual revert string (a Tenderly fork, or a recorded ContractCustomError from a de-whitelisted factory).

The rest of the coverage picture is unchanged: _deploy_service_onchain_from_safe carries # pragma: no cover (manage.py:438), so coverage tooling won't flag the gap either. CLAUDE.md requires a Tenderly-backed integration test for transaction-flow changes; the precedent for this exact mega-batch flow is tests/test_manage_update_batch_integration.py. If Task B is dropped this all becomes moot.

4. Findings independent of Task B

• tests/test_manage_unit.py:1620 is tautological. assert POLY_SAFE_SERVICE_NAMES == frozenset() restates constants.py:102 and catches only a revert of that one line. The behavioural assertion is that get_deploy_data_from_safe receives use_poly_safe=False for a polymarket_trader service — currently unreachable because the enclosing method is pragma: no cover. Either make that assertion reachable or drop the test; as written it's a change-detector.
• constants.py:102 — annotate frozenset[str] rather than bare frozenset.
• protocol.py:1600-1614 is now dead. With the constant empty, use_poly_safe can never be True, so get_poly_safe_deployment_payload and POLY_SAFE_CREATOR_WITH_RECOVERY_MODULE_CONTRACT are unreachable. Fine to leave for a follow-up, but worth a tracking issue rather than silent dead code.

Confirmed correct

Existing services are genuinely unaffected by Task A, as the description claims: they take the reuse_multisig=True branch of get_deploy_data_from_safe, which never consulted use_poly_safe (protocol.py:1559-1593). New deployments correctly fall through to safe_multisig_with_recovery_module.

Note on the latest two commits

d72d092c replaces str(e) in the cli.py handler with a hardcoded string. That message now exists in two places — manage.py:1010-1014 ("...is not whitelisted on-chain. Contact support.") and cli.py:1649 (same sentence, no "Contact support") — which will drift. It also leaves UnauthorizedMultisigException.__str__ with no remaining caller, and that override was already a no-op: Exception("boom") alone yields str(e) == "boom" when a single arg is passed, so both __init__ and __str__ on that class can go. All moot if Task B is dropped.

[valory-coding-agent]

Addressed 4 review comment(s):

• manage.py:1009 — Removed the UnauthorizedMultisig substring match block and all Task B code (exception class, CLI handler, revert attribution detection) per reviewer request. Task A (empty POLY_SAFE_SERVICE_NAMES) is preserved.
• test_manage_unit.py:1721 — Removed the TestRevertAttributionUnauthorizedMultisig test with the invented fixture.
• test_manage_unit.py:1621 — Dropped the tautological TestPolySafeServiceNames constant test.
• constants.py:102 — Annotated as frozenset[str].

Also removed TestUnauthorizedMultisigException and test_deploy_service_unauthorized_multisig (dead test code after Task B removal).

All lint, type, security, and unit test checks pass.

Re-triggering review.

[jmoreira-valory]

Review round 1 — ✅ Approve

What this does. Prepares the middleware for PolySafe being de-whitelisted on Polygon by emptying POLY_SAFE_SERVICE_NAMES, so new polymarket_trader deployments use the standard safe_multisig_with_recovery_module factory instead of the PolySafe factory. The originally-proposed Task B (translating the UnauthorizedMultisig revert into a structured HTTP 400) was dropped after review, leaving a one-line change.

Verified the change end-to-end at HEAD:

• Both consumers of POLY_SAFE_SERVICE_NAMES (manage.py:932 and manage.py:1153) are pure membership checks, so use_poly_safe is now always False.
• In protocol.py:get_deploy_data_from_safe, use_poly_safe is only consulted in the new-multisig branch; when reuse_multisig=True it is ignored entirely — so the PR body's claim that existing services with an already-deployed Safe are unaffected holds.
• New polymarket_trader deploys land in the use_recovery_module and not use_poly_safe branch → SAFE_MULTISIG_WITH_RECOVERY_MODULE_CONTRACT, exactly the intended path.
• No Task B remnants at HEAD: operate/exceptions.py contains only the pre-existing InsufficientFundsException; the prior CodeQL information-exposure alert on cli.py referred to the dropped handler and is moot.
• The frozenset[str] annotation requested by @OjusWiZard is in place. CI is green (21/21 checks).

Dropping Task B was the right call — "UnauthorizedMultisig" in str(ContractCustomError) could never match the raw ABI payload, as @OjusWiZard demonstrated.

Findings — 2 · 🔵 1 Low · ⚪ 1 Nit

#	Sev	Location	Issue
F1	🔵 LOW	operate/constants.py	PR description still documents the dropped Task B
F2	⚪ NIT	operate/services/protocol.py:1600	PolySafe deploy machinery is now unreachable — track a follow-up cleanup

---

Additional notes (not anchored to a diff line):

• 🔵 F1 · LOW — PR description still documents the dropped Task B The PR body describes UnauthorizedMultisigException, the manage.py revert-attribution change, the cli.py 400 mapping, and their unit tests — none of which exist at HEAD (the diff is 1 line in constants.py). Once merged, the description becomes the permanent record of the change and will mislead anyone doing archaeology on the PolySafe migration (e.g. searching for where UNAUTHORIZED_MULTISIG was introduced). Please trim the body to Task A before merging, optionally noting that Task B was dropped per review.
• ⚪ F2 · NIT — PolySafe deploy machinery is now unreachable — track a follow-up cleanup With POLY_SAFE_SERVICE_NAMES permanently empty, use_poly_safe is always False, so the use_recovery_module and use_poly_safe branch here, get_poly_safe_deployment_payload, and the POLY_SAFE_CREATOR_WITH_RECOVERY_MODULE_CONTRACT config are dead code. Keeping them during the transition is reasonable (rollback stays a one-line revert until de-whitelisting actually executes on-chain), but once the factory is de-whitelisted the code can never work again — worth a linked follow-up ticket to remove the PolySafe path and the constant itself.