Skip to content

feat: enable simplified BYC two-sided market flow#54

Merged
hoffmang9 merged 10 commits intomainfrom
feat/byc-two-sided-market-simplification
Mar 6, 2026
Merged

feat: enable simplified BYC two-sided market flow#54
hoffmang9 merged 10 commits intomainfrom
feat/byc-two-sided-market-simplification

Conversation

@hoffmang9
Copy link
Owner

Summary

  • implement true two-sided BYC<>wUSDC.b execution by carrying explicit buy/sell side through strategy planning, daemon execution, and cloud-wallet offer creation
  • simplify BYC market config to fixed_quote_per_base: 0.999, enable the market, and set config-driven 1-buy/3-sell size-10 targets with small buffers
  • preserve current zero-fee CAT split behavior and add clear side/pricing/inventory hook boundaries for future advanced model integration

Test plan

  • PATH="$(pwd)/.venv/bin:$PATH" pytest -q tests/test_config_models.py tests/test_daemon_strategy_integration.py tests/test_manager_post_offer.py tests/test_daemon_offer_execution.py tests/test_strategy.py
  • PATH="$(pwd)/.venv/bin:$PATH" pre-commit run --all-files

Made with Cursor

Implement true two-sided BYC<>wUSDC.b execution with side-aware offer construction and quote-side buy bootstrapping, while simplifying market pricing config to fixed quote-per-base. This keeps CAT split fee behavior at zero and leaves clear side/pricing/inventory hook boundaries for future advanced models.

Made-with: Cursor
Remove the temporary CAT split zero-fee override so coin-split uses the standard advised/config fallback fee path again, and update manager tests to match restored fee behavior.

Made-with: Cursor
@ibutterbot
Copy link

PR #54 Review - feat: enable simplified BYC two-sided market flow

Scope reviewed

  • PR metadata, commit history, and full diff across 10 changed files.
  • Implementation paths touched in greenfloor/daemon/main.py, greenfloor/cli/manager.py, greenfloor/config/models.py, and related tests.
  • Behavioral focus areas: two-sided action planning, buy/sell offer leg inversion, cloud-wallet reservation/bootstrapping, and config validation shifts.

Findings (ordered by severity)

1) High: buy-side bootstrap wait tracks the wrong asset ID

Where: greenfloor/cli/manager.py, _ensure_offer_bootstrap_denominations()

The function correctly switches the split target asset to quote-side for buy actions (split_asset_id = resolved_quote_asset_id), but _wait_for_mempool_then_confirmation(...) is still called with asset_id=resolved_base_asset_id.

Why this matters:

  • For buy-side bootstrap, new split outputs are quote-asset outputs, not base-asset outputs.
  • Waiting on base asset can produce false timeout/failure (bootstrap_wait_failed) even if the split succeeded.
  • That failure path enables fallback behavior and could trigger unnecessary/redundant cloud-wallet splitting, extra latency, and confusing operator guidance.

Suggested fix:

  • Pass asset_id=split_asset_id to _wait_for_mempool_then_confirmation(...) so wait logic tracks the same asset that was split.

Suggested test:

  • Add a buy-side bootstrap test where resolved_base_asset_id != resolved_quote_asset_id, assert wait is invoked with quote asset ID and that success/failure behavior maps correctly.

2) Medium: side attribution can silently drift to sell when execution metadata falls out of history window

Where: greenfloor/daemon/main.py, _active_offer_counts_by_size_and_side() and _recent_offer_metadata_by_offer_id()

Side/size are recovered from recent audit events with a hard cap (limit=1500). For active offer IDs lacking recent execution metadata, side defaults to "sell":

  • metadata = metadata_by_offer_id.get(offer_id)
  • side = metadata[1] if metadata is not None else "sell"

Why this matters:

  • In long-running deployments with high churn, older still-active buy offers can be misclassified as sell.
  • Misclassification skews counts_by_side, which drives replenishment planning in _evaluate_two_sided_market_actions().
  • Outcome can be systematic buy over-posting (buy undercount) or sell under-posting.

Suggested mitigations:

  • Persist side in offer state at creation/post time, and prefer state lookup over event reconstruction.
  • At minimum, treat unknown side as unmapped (not "sell"), increment a dedicated metric, and avoid directional assumptions.

Suggested tests:

  • Simulate active buy offers with missing execution metadata and verify they do not silently count as sell.
  • Add a test that asserts unknown-side handling behavior and resulting planned actions.

3) Medium: stable-quote validation now accepts invalid strategy fields silently

Where: greenfloor/config/models.py, _validate_strategy_pricing()

For quote_asset_type == "stable", validation now skips:

  • strategy_target_spread_bps
  • strategy_min_xch_price_usd
  • strategy_max_xch_price_usd

The new test intentionally confirms acceptance of values like 0, -1, and "invalid".

Why this matters:

  • This is likely intentional for minimal stable configs, but it broadens accepted config surface significantly.
  • Invalid fields can sit undetected and later become runtime hazards if code paths evolve or if market type/quote type is changed.

Suggested mitigation:

  • Keep fields optional for stable quotes, but if present, still validate type/range (or emit warnings) instead of fully bypassing checks.

Suggested tests:

  • Add explicit tests for "stable + field present but invalid" behavior based on desired policy (warn vs error vs ignore).

4) Low: test file contains accidental repeated assignment noise

Where: tests/test_manager_post_offer.py

prog.home_dir = str(tmp_path) is repeated multiple times in one test setup block, which appears accidental and reduces readability.

Impact:

  • No behavior change, but increases review noise and maintenance friction.

Suggested cleanup:

  • Collapse duplicated assignments to a single line per setup.

Style and maintainability notes

  • The side-normalization helper pattern (_normalize_offer_side) is consistently applied and improves guardrails.
  • Two-sided action planning extraction (_evaluate_two_sided_market_actions) is clean and easier to reason about than inlined branching.
  • Some naming remains sell-centric in helper APIs (plan_bootstrap_mixed_outputs(... sell_ladder=...)) even when used for buy-side quote splitting; consider renaming for clarity in a follow-up.

Security and performance considerations

  • Security: No new obvious secret-handling or auth-surface regressions in this PR. Main risks are correctness and operational reliability rather than exploitability.
  • Performance: Additional event reconstruction for side metadata is linear over recent events and likely acceptable now, but reliance on capped history can degrade correctness under high throughput (see finding Fix/ci run sim harness #2).

Tests to add (priority list)

  1. Buy-side bootstrap wait uses quote asset ID (regression test for finding Fix live testnet workflow secret gating #1).
  2. Two-sided active counting with missing side metadata does not default to sell silently.
  3. Reservation/bootstrapping end-to-end buy-side integration with distinct base/quote asset IDs.
  4. Config validation policy tests for stable quote markets with present-but-invalid strategy fields.

Overall summary

This PR makes meaningful progress toward true two-sided market flow and includes useful test updates, but there is one high-confidence correctness bug in buy-side bootstrap waiting plus a medium-risk side-accounting robustness gap under event-history truncation. I would not merge as-is without fixing the bootstrap wait asset mismatch and adding at least one targeted regression test for buy-side bootstrap tracking.

Use the split target asset for buy-side bootstrap confirmation waits, avoid defaulting unknown active-offer side to sell, and restore validation of present strategy pricing fields for stable quote markets. Add regression tests for each reviewed path and clean duplicated test setup noise.

Made-with: Cursor
@hoffmang9
Copy link
Owner Author

Addressed the review feedback with follow-up commit e2a6bd2.

Changes made

  1. Buy-side bootstrap wait asset fix (high)

    • In _ensure_offer_bootstrap_denominations(...), wait tracking now uses split_asset_id instead of always resolved_base_asset_id.
    • This ensures buy-side quote-asset splits are waited/confirmed against the correct asset.
  2. Unknown side metadata no longer defaults to sell (medium)

    • In _active_offer_counts_by_size_and_side(...), active offers missing recent execution metadata are now counted as unmapped (and skipped for side buckets) instead of implicitly treated as sell.
    • This prevents directional skew in two-sided replenishment logic when metadata is absent.
  3. Stable quote validation policy tightened (medium)

    • _validate_strategy_pricing(...) now validates strategy spread/price-band fields when present, including stable-quote markets.
    • This avoids silently accepting invalid config values.
  4. Test/readability follow-ups

    • Added regression tests for:
      • buy-side bootstrap wait tracking quote asset,
      • unknown-side active-offer accounting behavior,
      • stable-quote strategy-field validation when fields are present.
    • Removed repeated prog.home_dir = str(tmp_path) setup noise in manager tests.

Validation

  • pytest -q tests/test_manager_post_offer.py tests/test_daemon_offer_execution.py tests/test_config_models.py (200 passed)
  • pre-commit run --all-files (all hooks passed)

@ibutterbot
Copy link

Thanks — bootstrap wait changes look solid (nice: explicit bootstrap_wait_failed + buy waits on quote asset, and tests cover it).

One remaining correctness gap re: side accounting: _normalize_offer_side() returns 'sell' for anything except explicit 'buy', and _recent_offer_metadata_by_offer_id() normalizes item.get('side') through that. So malformed/missing side inside an existing metadata item still becomes sell (sell-biased fallback). That can undercount buys / skew two-sided planning over time.

Also noticed manager-written strategy_offer_execution items in _build_and_post_offer_cloud_wallet(...) appear to omit 'side'; if those are later consumed for accounting, they’ll hit the same sell-default path.

Request before full approve:

  • Make side parsing fail-closed (buy|sell|unknown/None) and treat unknown as unmapped (not sell).
  • Persist side consistently in strategy_offer_execution items when known.
  • Add a regression test for malformed/absent side in metadata staying unmapped + a test that manager audit items include side.

Treat malformed or missing side metadata as unknown/unmapped instead of sell-biased defaults, and ensure manager-written strategy_offer_execution items always include side when known. Add regression coverage for malformed side handling and manager audit-side persistence.

Made-with: Cursor
@hoffmang9
Copy link
Owner Author

Follow-up comment addressed in commit 845b38c.

What changed

  • Fail-closed metadata side parsing

    • Added strict side parsing for audit metadata (buy|sell|None).
    • Malformed/missing side in metadata is now treated as unknown and counted as unmapped, not as sell.
  • No sell-biased fallback in side-aware counting

    • _active_offer_counts_by_size_and_side(...) now requires valid side metadata to attribute directional counts.
    • Unknown-side active offers remain visible via unmapped counters and do not skew buy/sell planning.
  • Manager now persists side in execution audit items

    • _build_and_post_offer_cloud_wallet(...) now writes side into strategy_offer_execution.items and normalizes action_side once for consistency across phases.

Tests added

  • malformed/missing side metadata stays unmapped (tests/test_daemon_offer_execution.py)
  • manager strategy execution audit items include side (sell and buy paths) (tests/test_manager_post_offer.py)

Validation

  • pytest -q tests/test_manager_post_offer.py tests/test_daemon_offer_execution.py (164 passed)
  • pre-commit run --all-files (all hooks passed)

@ibutterbot
Copy link

ibutterbot commented Mar 4, 2026

Re-review after your follow-up (commit 845b38c): this addresses the remaining side-accounting concerns I called out.

  • Side parsing is now fail-closed (buy|sell|None) and malformed/missing side stays unmapped (no sell-biased default).
  • Side-aware counting requires valid side metadata; unknown-side offers don’t skew buy/sell buckets.
  • Manager now persists side into strategy_offer_execution items, and you added targeted regression tests for both the daemon metadata path and the manager audit path.

Assuming CI passes the full suite, this looks good to me.

Document the PR follow-up hardening commit and verification, including John-Deere fast-forward to 845b38c and daemon restart on that runtime revision.

Made-with: Cursor
Cap daemon coin-op combine submissions at 5 inputs, add exponential backoff retries for Cloud Wallet 429 responses, and use capped progress combines in split prerequisites with explicit operator-facing notes.

Made-with: Cursor
Update manager cloud-wallet polling and Dexie visibility validation flows, wire the adapter support they require, and refresh runbook and regression tests to match the new behavior.

Made-with: Cursor
Move cloud-wallet offer posting and asset resolution out of manager internals so the daemon can share one runtime path, while keeping test seams intact. Also record the ent-wallet BYC query issue and add temporary client-side mitigations for scoped coin queries and sub-unit CAT coin operations.

Made-with: Cursor
Reject non-1000 CAT unit multipliers during market config parsing and default missing CAT multipliers to the canonical 1000-mojo unit. This prevents host-specific config drift from silently mispricing CAT offers.

Made-with: Cursor
Revalidate scoped CAT coin candidates by direct CoinRecord lookup before split or combine selection so cross-asset rows leaked by Cloud Wallet cannot be treated as spendable inventory. Record the live John-Deere evidence and note that this defense is temporary until the upstream scoped-query bug is fixed.

Made-with: Cursor
@hoffmang9 hoffmang9 merged commit 5754a5e into main Mar 6, 2026
3 checks passed
@hoffmang9 hoffmang9 deleted the feat/byc-two-sided-market-simplification branch March 6, 2026 01:21
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