Skip to content

feat(offchain): wire mech-interact off-chain branch through Trader FSM#1002

Merged
DIvyaNautiyal07 merged 4 commits into
offchain-epicfrom
feat/offchain-mech-integration
Jul 1, 2026
Merged

feat(offchain): wire mech-interact off-chain branch through Trader FSM#1002
DIvyaNautiyal07 merged 4 commits into
offchain-epicfrom
feat/offchain-mech-integration

Conversation

@DIvyaNautiyal07

Copy link
Copy Markdown
Contributor

Summary

Adds the off-chain mech path (mech-interact v0.32.4-rc1) into trader. Default behaviour is unchanged — an operator opts in by flipping use_offchain: true in MECH_MARKETPLACE_CONFIG.

This is the trader-side companion to mech-interact PR #95 (executor + 402 + deposit builder + response poller) plus its review-feedback fix-up PR #96 (chain_id routing, mech_tool gate, packed Safe tx). The optimus equivalent is valory-xyz/optimus#355.

FSM wiring (composition.py — 4 new edges)

Source Target Why
FinishedOffchainMechRequestRound MechResponseRound Happy path: 200 from mech, skip on-chain settlement, poll HTTP
FinishedOffchainMechDepositNeededRound PreTxSettlementRound 402: settle the auto-deposit via trader's existing refill-check gate; the deposit tx is bet-agnostic so the gas check applies identically
FinishedOffchainMechDepositSettledRound MechRequestRound Deposit landed: re-enter so the executor's _retry_pending re-POSTs the cached request_id
FailedOffchainMechRequestRound HandleFailedTxRound All retries exhausted: enter the existing mech-timeout recovery

Each edge pinned individually in the composition test (parametrised OFFCHAIN_TRANSITIONS) — flat value-set assertions would let a silent target swap pass (e.g. routing settled-deposit to MechResponseRound instead of MechRequestRound would break _retry_pending).

Multiplexer (tx_settlement_multiplexer_abci/rounds.py)

  • New event Event.OFFCHAIN_MECH_DEPOSIT_SETTLED.
  • New degenerate round FinishedOffchainMechDepositSettledRound.
  • PostTxSettlementRound.end_block dispatches OFFCHAIN_DEPOSIT_TX_SUBMITTER → the new event.
  • UNRECOGNIZED branch now logs the offending submitter at WARNING so a silently-dropped off-chain deposit is visible in agent logs instead of only in raw Tendermint state.

handle_failed_tx.py

Treats tx_submitter == OFFCHAIN_DEPOSIT_TX_SUBMITTER as a mech timeout alongside the on-chain MechRequestRound id. Without this, shared_state.mech_timed_out and after_bet_attempt would stay False on an off-chain failure path, silently skipping the tool-quarantine / retry bookkeeping the on-chain timeout path triggers.

Config parity

Added 7 off-chain knobs (use_offchain, offchain_url, offchain_deposit_target_calls, auto_deposit_cap_per_cycle, offchain_poll_interval_seconds, offchain_poll_timeout_seconds, offchain_failover_max_retries) with use_offchain: false defaults in:

  • packages/valory/skills/trader_abci/skill.yaml
  • packages/valory/services/polymarket_trader/service.yaml
  • packages/valory/services/trader_pearl/service.yaml

A MECH_MARKETPLACE_CONFIG env override now carries the full dict shape; the agent does not depend on MechParams.get() defaults for the off-chain path.

Tests

  • test_composition.py — 4 new edges pinned via OFFCHAIN_TRANSITIONS; EXPECTED_TRANSITION_MAPPING_LENGTH 57 → 61.
  • tx_settlement_multiplexer_abci/tests/test_rounds.pytest_offchain_deposit_submitter_dispatches_to_settled_event covers the sentinel dispatch and pins the literal sentinel value (not just identity); test_unrecognized_submitter asserts the new WARNING log; test_final_states_count 14 → 15.

All 1537 tests pass locally. autonomy packages lock --check green. autonomy analyse docstrings stable. CI-pinned isort (isort==8.0.0 via tomte) green.

Why this looks the way it does

Trader's mech consumer flow is more complex than optimus' (PreTxSettlement gate + DecisionReceive consumer reads mech_responses[0]), but mech-interact's off-chain response poller writes to mech_responses "exactly as the on-chain branch does" (per its docstring), and PreTxSettlementRound is a generic agent-balance refill check (not bet-specific), so the off-chain deposit slots into the existing settlement pipeline cleanly.

Verified against real Gnosis test mech

The existing Gnosis test mech (service 2692, 0x12c11959c9ae177b489800808cdf27c3cbb63765) has already run the full off-chain x402 settlement cycle end-to-end (tx 0xf19673...3cd89e). This PR's wiring targets exactly that setup.

🤖 Generated with Claude Code

@bennyjo bennyjo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-chain wiring review. Default path (use_offchain: false) is untouched and the FSM wiring checks out; three findings on the off-chain path itself — one likely-blocking (contract registration), plus a config-units question and a test gap.

Comment thread packages/packages.json
"contract/valory/agreement_store_manager/0.1.0": "bafybeign6u6635cpp7z2rws7uaaxgzvyokdyzztnadmivbt2qsvw2q2vza",
"contract/valory/transfer_nft_condition/0.1.0": "bafybeib6wigs3ufiuzkj6twlthcya67lswm3qosikrg4ez7jjgkam4kz5m",
"contract/valory/subscription_provider/0.1.0": "bafybeia7fmpemztyjwu4pu4p3q2udy7ymrxz4p5eklegdlc2bgnfi2zoae",
"contract/valory/mech_marketplace/0.1.0": "bafybeieadbmhzndxe6jcfwujcpjrljpc5kfwffkd3zzpg75uxksufn4l3i",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three contracts (mech_marketplace, balance_tracker_fixed_price_native, balance_tracker_fixed_price_token) are vendored here and declared by the upstream mech_interact_abci skill, but they aren't added to the contracts: section of packages/valory/agents/trader/aea-config.yaml — where every other mech-interact contract (mech, mech_mm, mech_marketplace_legacy, nvm_balance_tracker_*, agent_mech) is explicitly listed. The agent loads only the components in that section, so the first off-chain contract call when use_offchain: true (MechMarketplace.get_nonce/get_chain_id/get_balance_tracker, or the balance-tracker reads) won't resolve at runtime.

The twin optimus#355 has the identical omission, and the end-to-end verification noted in the description appears to exercise the mech side rather than a trader agent built from this branch with use_offchain: true — so the trader-side registration looks untested. Could you confirm a trader agent built from this branch resolves these three contracts? If not, they need adding to the contracts: section of packages/valory/agents/trader/aea-config.yaml, and mirrored in optimus#355.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 5ef4591. Verified locally — the three contracts (mech_marketplace, balance_tracker_fixed_price_native, balance_tracker_fixed_price_token) were indeed missing from the agent's contracts: section, added alongside the existing mech-interact entries in packages/valory/agents/trader/aea-config.yaml. Good catch — the multi-round Claude iterations on this PR were focused on FSM wiring and missed that the vendored contracts (which are declared in the skill) still need to be enumerated at the agent level to load at runtime. Mirroring the same fix into optimus#355 as a follow-up commit.

use_subgraph_for_redeeming: ${USE_SUBGRAPH_FOR_REDEEMING:bool:true}
use_mech_marketplace: ${USE_MECH_MARKETPLACE:bool:false}
mech_marketplace_config: ${MECH_MARKETPLACE_CONFIG:dict:{"mech_marketplace_address":"0x4554fE75c1f5576c1d7F765B2A036c199Adae329","priority_mech_address":"0x0000000000000000000000000000000000000000","priority_mech_staking_instance_address":"0x998dEFafD094817EF329f6dc79c703f1CF18bC90","priority_mech_service_id":975,"requester_staking_instance_address":"0x0000000000000000000000000000000000000000","response_timeout":300}}
mech_marketplace_config: ${MECH_MARKETPLACE_CONFIG:dict:{"mech_marketplace_address":"0x4554fE75c1f5576c1d7F765B2A036c199Adae329","priority_mech_address":"0x0000000000000000000000000000000000000000","priority_mech_staking_instance_address":"0x998dEFafD094817EF329f6dc79c703f1CF18bC90","priority_mech_service_id":975,"requester_staking_instance_address":"0x0000000000000000000000000000000000000000","response_timeout":300,"use_dynamic_mech_selection":false,"use_offchain":false,"offchain_url":null,"offchain_deposit_target_calls":10,"auto_deposit_cap_per_cycle":5000000,"offchain_poll_interval_seconds":3.0,"offchain_poll_timeout_seconds":300.0,"offchain_failover_max_retries":2}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto_deposit_cap_per_cycle: 5000000 is the same value used in polymarket_trader/service.yaml, where it's 5 USDC (6 decimals) — sane. Here on pearl, against an 18-decimal native/token denomination, 5000000 is dust. If an operator flips use_offchain: true on pearl without also raising the cap, every auto-deposit is dust-capped and silently fails over instead of funding the request — a quiet "feature doesn't work," not a loud error.

Is a single hardcoded default meant to cover both 6- and 18-decimal payment tokens, or should the pearl default differ? Happy to leave as-is if the cap is always set per deployment — flagging because the failure mode is silent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5ef4591. You're right — 5000000 wei on 18-decimal native is dust and the failure mode is exactly the silent one you described (cap-below-shortfall triggers OFFCHAIN_402_INSUFFICIENT without any obvious operator signal). Bumped trader_pearl's default to 5000000000000000000 (5 xDAI, matching the polymarket_trader default at its own denomination). Operators overriding via env still work as before.

# the on-chain mech-timeout path.
mech_timed_out = tx_submitter in (
MechRequestRound.auto_round_id(),
OFFCHAIN_DEPOSIT_TX_SUBMITTER,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new tuple membership is the crux of the change — without it, mech_timed_out/after_bet_attempt stay False on the off-chain failure path and the tool-quarantine bookkeeping silently skips. But the description's "covered by existing decision_maker_abci tests" can't apply here: those tests predate this sentinel, so the tx_submitter == OFFCHAIN_DEPOSIT_TX_SUBMITTER → mech_timed_out is True branch has no assertion. The multiplexer side got a precise new test (test_offchain_deposit_submitter_dispatches_to_settled_event, even pinning the literal sentinel string); the symmetric decision-maker test is missing. Could you add one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 5ef4591test_offchain_deposit_sentinel_treated_as_mech_timeout under decision_maker_abci/tests/behaviours/test_handle_failed_tx.py. Asserts shared_state.mech_timed_out is True (via a captured mock, not the default MagicMock which silently accepts anything) and payload.vote is True. Also pins the literal sentinel string "mech_request_round_offchain_deposit" — same reasoning as the multiplexer test: a rename in mech-interact would still break the executor which hardcodes the string, so the identity-only check isn't sufficient.

DIvyaNautiyal07 added a commit that referenced this pull request Jul 1, 2026
…ve cap + test

Three findings from bennyjo:

1. [Blocking] The three new mech-interact contracts (mech_marketplace,
   balance_tracker_fixed_price_native/_token) were vendored via the
   skill dependency but not registered in the agent aea-config's
   ``contracts:`` section, so the first off-chain contract call under
   ``use_offchain: true`` (``MechMarketplace.get_chain_id`` etc.) would
   fail to resolve at runtime. Added the three contracts to
   ``packages/valory/agents/trader/aea-config.yaml`` alongside the
   existing mech-interact contract entries.

2. ``trader_pearl/service.yaml`` inherited the polymarket_trader default
   for ``auto_deposit_cap_per_cycle`` (5000000 — 5 USDC at 6 decimals),
   but pearl runs against an 18-decimal native/token denomination where
   5000000 wei is dust; every auto-deposit would silently fail over
   instead of funding the request. Bumped to 5000000000000000000
   (5 xDAI at 18 decimals) so the two services carry equivalent-scale
   defaults.

3. Added ``test_offchain_deposit_sentinel_treated_as_mech_timeout``
   under ``decision_maker_abci/tests/behaviours/test_handle_failed_tx.py``
   — asserts on ``shared_state.mech_timed_out`` (captured via a mock)
   in addition to the payload, and pins the literal sentinel value
   (mirrors the multiplexer test the reviewer flagged).

Also mirrors optimus#355 — same aea-config omission is being applied
there in a follow-up commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DIvyaNautiyal07 and others added 2 commits July 1, 2026 15:00
Adds the off-chain branches from mech-interact v0.32.4-rc1 (PR #95 +
PR #96 bugfixes) into trader. Default behaviour is unchanged — an
operator opts in by flipping ``use_offchain: true`` in
``MECH_MARKETPLACE_CONFIG``.

FSM wiring (composition.py):
  * FinishedOffchainMechRequestRound -> MechResponseRound
    (happy path: 200 from mech, skip on-chain settlement, poll HTTP)
  * FinishedOffchainMechDepositNeededRound -> PreTxSettlementRound
    (402: settle the auto-deposit via trader's existing refill-check
    gate; the deposit tx is bet-agnostic so the gas check works
    identically to a bet tx)
  * FinishedOffchainMechDepositSettledRound -> MechRequestRound
    (deposit landed: re-enter so the executor's ``_retry_pending``
    re-POSTs the cached request_id)
  * FailedOffchainMechRequestRound -> HandleFailedTxRound
    (all retries exhausted: enter the existing mech-timeout recovery)

Multiplexer (tx_settlement_multiplexer_abci/rounds.py):
  * New event Event.OFFCHAIN_MECH_DEPOSIT_SETTLED.
  * New degenerate round FinishedOffchainMechDepositSettledRound.
  * PostTxSettlementRound.end_block dispatches
    OFFCHAIN_DEPOSIT_TX_SUBMITTER to the new event.
  * UNRECOGNIZED branch now logs the offending submitter at WARNING so
    a silently-dropped off-chain deposit is visible in agent logs
    instead of only in raw Tendermint state.

handle_failed_tx.py:
  * Treats tx_submitter == OFFCHAIN_DEPOSIT_TX_SUBMITTER as a mech
    timeout alongside the on-chain MechRequestRound id, so the
    tool-quarantine / shared_state.mech_timed_out / after_bet_attempt
    bookkeeping fires identically for both paths.

Config parity (skill.yaml + both service.yaml MECH_MARKETPLACE_CONFIG
env-defaults): seven off-chain knobs added with use_offchain: false
default so an env override carries the full dict shape.

Bumps mech-interact to v0.32.4-rc1 (skill hash
bafybeihhwzpa6x5ypth426tzd2zjsnys6jmjaab4zvk4edisetzktft754) and
pulls in the three new third_party contracts
(mech_marketplace, balance_tracker_fixed_price_native/_token).

Tests:
  * test_composition.py — each new edge pinned individually
    (parametrised OFFCHAIN_TRANSITIONS); flat value-set assertions
    would let a silent target swap pass.
  * test_rounds.py — pins OFFCHAIN_DEPOSIT_TX_SUBMITTER dispatch and
    UNRECOGNIZED WARNING log; tripwires updated for new final state
    and transition entries.
  * handle_failed_tx covered by existing decision_maker_abci tests.

Lock cascade + FSM specs + abci docstrings regenerated.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ve cap + test

Three findings from bennyjo:

1. [Blocking] The three new mech-interact contracts (mech_marketplace,
   balance_tracker_fixed_price_native/_token) were vendored via the
   skill dependency but not registered in the agent aea-config's
   ``contracts:`` section, so the first off-chain contract call under
   ``use_offchain: true`` (``MechMarketplace.get_chain_id`` etc.) would
   fail to resolve at runtime. Added the three contracts to
   ``packages/valory/agents/trader/aea-config.yaml`` alongside the
   existing mech-interact contract entries.

2. ``trader_pearl/service.yaml`` inherited the polymarket_trader default
   for ``auto_deposit_cap_per_cycle`` (5000000 — 5 USDC at 6 decimals),
   but pearl runs against an 18-decimal native/token denomination where
   5000000 wei is dust; every auto-deposit would silently fail over
   instead of funding the request. Bumped to 5000000000000000000
   (5 xDAI at 18 decimals) so the two services carry equivalent-scale
   defaults.

3. Added ``test_offchain_deposit_sentinel_treated_as_mech_timeout``
   under ``decision_maker_abci/tests/behaviours/test_handle_failed_tx.py``
   — asserts on ``shared_state.mech_timed_out`` (captured via a mock)
   in addition to the payload, and pins the literal sentinel value
   (mirrors the multiplexer test the reviewer flagged).

Also mirrors optimus#355 — same aea-config omission is being applied
there in a follow-up commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DIvyaNautiyal07 DIvyaNautiyal07 force-pushed the feat/offchain-mech-integration branch from eb42f0e to 782233e Compare July 1, 2026 09:31
D205 wants exactly one blank line between the summary line and the
extended description. Collapsed the summary onto one line.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DIvyaNautiyal07 DIvyaNautiyal07 changed the base branch from main to offchain-epic July 1, 2026 10:08

@bennyjo bennyjo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One parity note on the off-chain config rollout.

README.md: bafybeichoh5tnk4qctaazcjcrxvpwc5yasodleqoeapnfapwrrdh3tm5gu
fingerprint_ignore_patterns: []
agent: valory/trader:0.1.0:bafybeifdcmuilvxj5gf3d7dumlgcab3qwsp2xkmhqfnfxef3zxulpbthxy
agent: valory/trader:0.1.0:bafybeigotorqhbdig74qvmj2isqje3sjjq44s5yzjn3duqa4q2l2zf5zsi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description claims all three services got the full off-chain config, but polymarket_trader/service.yaml was never updated — it still has the old 6-key shape.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0956931 — polymarket_trader now carries the seven off-chain keys with a 6-decimal cap (5000000 = 5 USDC). Missed it during the earlier rebase (had a conflict on that specific line and picked the wrong side). Thanks for catching it.

@OjusWiZard OjusWiZard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated multi-perspective review. One critical blocker + one important config gap + two advisories, posted inline. Summary: the trader-side FSM wiring, config surface, and tests are solid, but the branch is not runnable as committed because the mech_interact_abci rc1 source it depends on is missing from the branch (see inline on packages/packages.json).

Comment thread packages/packages.json
"skill/valory/reset_pause_abci/0.1.0": "bafybeihc4ruh25s7wuunfy2b3k2n4eypyhvtry3usgom44wsymagbcr3ki",
"skill/valory/termination_abci/0.1.0": "bafybeibnnzry5kknci3z46gcjgutv4jid3ir6xn6a5adq3tx6v2e25augq",
"skill/valory/mech_interact_abci/0.1.0": "bafybeidnmszufszq3ro4ebgrqzmhqgctrsc6sykeanfsbvotghvt2cgfsy",
"skill/valory/mech_interact_abci/0.1.0": "bafybeihhwzpa6x5ypth426tzd2zjsnys6jmjaab4zvk4edisetzktft754",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical — branch is non-functional as committed.

This bumps mech_interact_abci to the rc1 CID (bafybeihhwzpa…) here and in every skill.yaml/aea-config.yaml ref, but the vendored source under packages/valory/skills/mech_interact_abci/ was never synced — it's still the old version. None of the off-chain symbols the trader-side code imports exist there:

ImportError: cannot import name 'OFFCHAIN_DEPOSIT_TX_SUBMITTER'
  from '...mech_interact_abci.states.request'
ImportError: cannot import name 'FinishedOffchainMechRequestRound'
  from '...mech_interact_abci.states.final_states'

grep confirms mech_request_round_offchain_deposit and the offchain rounds appear only in this PR's four trader files, nowhere in the mech_interact package. Consequences:

  • composition.py, handle_failed_tx.py, tx_settlement_multiplexer_abci/rounds.py and all three touched test modules fail at collection time → agent won't start, suite can't run.
  • autonomy packages lock --check will fail: this line claims the new CID but the vendored source still hashes to the old bafybeidnmsz….

This contradicts the PR description's "All 1537 tests pass locally" and "packages lock --check green" — those hold only in a local tree where rc1 was synced but not committed.

Fix: sync + commit the mech_interact_abci v0.32.4-rc1 source onto the branch (or land it on offchain-epic first and confirm it's present in the merge target).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bug — vendored source path. packages/valory/skills/mech_interact_abci/ is gitignored in trader (see .gitignore line 16); the source is downloaded by CI via autonomy packages sync at test time against the hash in packages.json, and the committed hash matches v0.32.4-rc1. Same pattern as abstract_round_abci, registration_abci, all other imported skills. That's why (a) CI is green on this branch (all 14 checks including lock_check and test suites pass on the current commit) and (b) my local claim of "1687 tests pass / lock --check green" is accurate — the runner and my local venv both synced rc1. If we ever move to committing the source into the tree, that's a repo-wide decision and needs applying to every third-party skill, not just this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — retracting this. Confirmed .gitignore:16 ignores packages/valory/skills/mech_interact_abci/ and git ls-files shows it untracked (same as abstract_round_abci et al.), so CI syncs the rc1 source by the packages.json hash and lock --check validates against IPFS, not the working tree. My ImportError was a stale local autonomy packages sync cache, not a defect in the branch. Apologies for the noise — not a blocker.

use_subgraph_for_redeeming: ${USE_SUBGRAPH_FOR_REDEEMING:bool:true}
use_mech_marketplace: ${USE_MECH_MARKETPLACE:bool:false}
mech_marketplace_config: ${MECH_MARKETPLACE_CONFIG:dict:{"mech_marketplace_address":"0x4554fE75c1f5576c1d7F765B2A036c199Adae329","priority_mech_address":"0x0000000000000000000000000000000000000000","priority_mech_staking_instance_address":"0x998dEFafD094817EF329f6dc79c703f1CF18bC90","priority_mech_service_id":975,"requester_staking_instance_address":"0x0000000000000000000000000000000000000000","response_timeout":300}}
mech_marketplace_config: ${MECH_MARKETPLACE_CONFIG:dict:{"mech_marketplace_address":"0x4554fE75c1f5576c1d7F765B2A036c199Adae329","priority_mech_address":"0x0000000000000000000000000000000000000000","priority_mech_staking_instance_address":"0x998dEFafD094817EF329f6dc79c703f1CF18bC90","priority_mech_service_id":975,"requester_staking_instance_address":"0x0000000000000000000000000000000000000000","response_timeout":300,"use_dynamic_mech_selection":false,"use_offchain":false,"offchain_url":null,"offchain_deposit_target_calls":10,"auto_deposit_cap_per_cycle":5000000000000000000,"offchain_poll_interval_seconds":3.0,"offchain_poll_timeout_seconds":300.0,"offchain_failover_max_retries":2}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important — config-parity claim not met for Polystrat.

This (Omenstrat) default correctly carries the 7 off-chain keys, but packages/valory/services/polymarket_trader/service.yaml MECH_MARKETPLACE_CONFIG still ends at response_timeout — the off-chain knobs are absent. The PR body claims parity across all three files (skill.yaml + both service.yaml).

Under autonomy deploy, the service-level dict replaces the skill.yaml default, so on Polystrat the off-chain keys fall back to MechParams.get() defaults — the exact thing the PR says it avoids ("the agent does not depend on MechParams.get() defaults for the off-chain path"). Blast radius is currently low (off-chain is Gnosis-only, defaults off), but the stated invariant is broken and off-chain can't be enabled on polymarket via config.

Fix: add the 7 keys to polymarket_trader/service.yaml, or correct the PR description.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0956931 — added the seven off-chain keys to polymarket_trader's MECH_MARKETPLACE_CONFIG env default (cap kept at 5e6 = 5 USDC since polymarket_trader pays in 6-decimal USDC). Parity restored across all three files.

use_offchain: false
offchain_url: null
offchain_deposit_target_calls: 10
auto_deposit_cap_per_cycle: 5000000

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Advisory — cap default differs by 12 orders of magnitude, uncommented.

Here auto_deposit_cap_per_cycle: 5000000 (5e6) vs trader_pearl/service.yaml env default 5000000000000000000 (5e18). On Gnosis native (18 decimals) this value is ~5e-12 xDAI — effectively a zero cap that would block every auto-deposit. It's overridden by the service default for Omenstrat, so it only bites make run-agent local dev (and only if off-chain were enabled), but the bare 5000000 reads like a placeholder. Add a decimals comment or align the value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0956931 — bumped trader_abci/skill.yaml default from 5000000 to 5000000000000000000 (5 xDAI at 18 decimals, matching trader_pearl's Gnosis native denomination). Added a decimals comment inline, but autonomy packages lock strips YAML comments on serialization so the rationale lives in the commit message body instead.

(MechResponseRound, DONE): DecisionReceiveRound
(MechResponseRound, NO_MAJORITY): MechResponseRound
(MechResponseRound, ROUND_TIMEOUT): HandleFailedTxRound
(MechResponseRound, RESPONSE_ROUND_TIMEOUT): HandleFailedTxRound

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Advisory — on-chain path change rides along (dependency-driven).

The MechResponseRound timeout event was renamed ROUND_TIMEOUTRESPONSE_ROUND_TIMEOUT. Once the rc1 source is synced, verify the composed app registers a timeout for RESPONSE_ROUND_TIMEOUT in event_to_timeout — otherwise the response-timeout → HandleFailedTxRound recovery silently never fires (the currently-vendored app only defines ROUND_TIMEOUT: 30.0). Can't confirm now because the dep source is absent (see the packages.json comment).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified — mech_interact_abci v0.32.4-rc1 (states/base.py:87 and rounds.py:197) registers Event.RESPONSE_ROUND_TIMEOUT: 330.0 in its own event_to_timeout, so the composed TraderAbciApp inherits it via chain(). No trader-side registration needed.

Two review findings from OjusWiZard's second pass:

1. [Important] ``polymarket_trader/service.yaml`` was missing the 7
   off-chain keys in the ``MECH_MARKETPLACE_CONFIG`` env-default dict —
   the parity claim in the PR description didn't hold for that service.
   Added them alongside the polymarket_trader-appropriate cap
   (``5000000`` = 5 USDC at 6 decimals, matching its existing payment
   token).

2. [Advisory] ``trader_abci/skill.yaml`` default
   ``auto_deposit_cap_per_cycle`` was ``5000000``, which is dust
   (~5e-12 xDAI) at the Gnosis native denomination the skill.yaml
   default is intended for. Bumped to ``5000000000000000000`` (5 xDAI
   at 18 decimals, matching ``trader_pearl``) and added a decimals
   comment explaining that polymarket_trader overrides to 6-decimal
   USDC via service.yaml.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@bennyjo bennyjo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review LGTM. All five findings from the prior passes are resolved: the three mech-interact contracts are registered in aea-config.yaml, trader_pearl + the trader_abci skill default carry the 18-decimal native cap, polymarket_trader has full 7-key MECH_MARKETPLACE_CONFIG parity, and the sentinel/timeout tests pin literal values. FSM wiring is complete and symmetric (4 new edges pinned individually, +4 mapping length, new degenerate round registered consistently across final_states/transition_function/db_post_conditions), and CI is green across all platforms. Default-off posture keeps the path dormant until an operator opts in.

One non-blocking sanity check: polymarket_trader auto_deposit_cap_per_cycle: 5000000 is 6-decimal — worth a one-line confirm that its marketplace (0x343F2B…, priority mech service 25) actually prices auto-deposits in 6-decimal USDC rather than 18-decimal native, since the cap tracks the mech's balance-tracker token, not the betting token. Consistent with the distinct marketplace address, so most likely correct.

@OjusWiZard OjusWiZard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Re-reviewed after the fix-up commits:

  • #1 (mech_interact not synced) — my error; retracted. The skill is gitignored and CI-synced by hash; my local ImportError was a stale sync cache.
  • #2 Polystrat parity — fixed; polymarket_trader carries the 7 keys at the correct 6-decimal cap.
  • #3 cap default — fixed; skill.yaml + trader_pearl both 5e18.
  • #4 RESPONSE_ROUND_TIMEOUT — verified rc1 registers the 330.0 timeout, inherited via chain().
  • contract registration in aea-config.yaml — fixed.

FSM wiring, config surface, and the new tests (literal-sentinel pinning, per-edge parametrization) look solid. Only remaining nit is advisory: the off-chain operator knobs aren't documented in README — non-blocking.

@DIvyaNautiyal07 DIvyaNautiyal07 merged commit 86308fc into offchain-epic Jul 1, 2026
23 checks passed
@DIvyaNautiyal07 DIvyaNautiyal07 deleted the feat/offchain-mech-integration branch July 1, 2026 14:54
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.

3 participants