Skip to content

ddo: clamp term-max via schedule.Duration; reject legacy price flags#666

Merged
parkan merged 4 commits into
data-preservation-programs:mainfrom
parkan:ddo-duration-and-price-guard
Apr 23, 2026
Merged

ddo: clamp term-max via schedule.Duration; reject legacy price flags#666
parkan merged 4 commits into
data-preservation-programs:mainfrom
parkan:ddo-duration-and-price-guard

Conversation

@parkan

@parkan parkan commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two small DDO-scheduling fixes that came up running a test SP end-to-end on calibnet.

1. Schedule-level `Duration` now influences DDO `TermMax`

Today `runDDOSchedule` uses `d.ddoSchedulingConfig.TermMax` as-is -- every DDO allocation submits with the daemon's configured term max, regardless of the schedule's `--duration`. This means an operator can't run a daemon with a permissive ceiling (e.g. 5y) while scheduling individual preparations at shorter terms.

Change: if `schedule.Duration` is non-zero, the local per-run `DDOSchedulingConfig` copy lowers `TermMax` to match. The daemon's `--ddo-term-max` still acts as a hard ceiling -- a longer schedule Duration cannot raise it. The daemon's `TermMin` is unchanged; if Duration falls below it, `Validate()` rejects the run (existing behaviour).

2. DDO schedules reject legacy price flags

The `--price-per-gb-epoch`, `--price-per-gb`, and `--price-per-deal` flags are meaningful only for the legacy boost/market path. For DDO, payment amounts come from the SP's on-chain registered price in the DDO contract (`EnsurePayments` reads it and deposits USDFC accordingly). Silently accepting these flags for DDO led to me assuming I could override the SP's price at schedule-create time, which I couldn't -- and there's no log line that says so.

Change: `CreateHandler` returns `ErrInvalidParameter` when any of the three price flags are non-zero on a DDO schedule.

Test plan

  • `TestCreateHandler_DDORejectsPricePerGBEpoch` (new)
  • `TestCreateHandler_DDORejectsPricePerGB` (new)
  • `TestCreateHandler_DDORejectsPricePerDeal` (new)
  • `TestDealPusher_RunSchedule_DDODurationClampsTermMax` (new) -- table-driven with four cases (below ceiling, at ceiling, above ceiling clamped, zero duration)
  • Existing `handler/deal/schedule` and `service/dealpusher` packages still green

parkan and others added 2 commits April 21, 2026 10:49
- runDDOSchedule: when schedule.Duration is non-zero, lower the local
  DDOSchedulingConfig.TermMax. The daemon --ddo-term-max remains the
  ceiling; a schedule-level duration can only shorten the term. The
  daemon config struct is copied per call, so this is scoped to the
  current run.

- CreateHandler: reject --price-per-gb-epoch / --price-per-gb /
  --price-per-deal for DDO schedules. DDO payment comes from the SP's
  on-chain registered price via EnsurePayments; those flags were
  silently ignored before, which made it easy to misconfigure.
@anjor

anjor commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

Code review

Found 1 issue:

  1. UpdateHandler does not mirror the new DDO price-flag rejection, so the create-time guard can be bypassed via PATCH. A user creates a DDO schedule with no prices (passes the new check at create.go:237), then updates it with --price-per-gb-epoch / --price-per-gb / --price-per-deal, and the write lands silently — reintroducing the exact "user assumes they overrode the SP price" failure mode this PR is trying to prevent. The updates map writes the three price fields with no effectiveDealType check, the same shape as the url_template invariant that was guarded in add DDO as third deal type #644.

updates["url_template"] = *request.URLTemplate
}
if request.PricePerGBEpoch != nil {
updates["price_per_gb_epoch"] = *request.PricePerGBEpoch
}
if request.PricePerGB != nil {
updates["price_per_gb"] = *request.PricePerGB
}
if request.PricePerDeal != nil {
updates["price_per_deal"] = *request.PricePerDeal
}

For reference, the create-time guard this is meant to mirror:

// DDO payment comes from the SP's on-chain registered price in the
// DDO contract -- the deal-pusher's EnsurePayments reads that and
// deposits USDFC accordingly. The price-per-* flags were for legacy
// market deals and are silently ignored otherwise; rejecting here
// keeps the user from assuming they can override the SP's price.
if request.PricePerGBEpoch != 0 || request.PricePerGB != 0 || request.PricePerDeal != 0 {
return nil, errors.Wrap(handlererror.ErrInvalidParameter,
"DDO schedules do not accept --price-per-gb-epoch / --price-per-gb / --price-per-deal; "+
"payment is determined by the SP's on-chain registered price in the DDO contract")
}

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

mirrors the create-time guard added in this PR. without it a user
could create a clean DDO schedule, then PATCH it with --price-per-*
and the write would land silently -- reintroducing the same 'i think
i overrode the SP price' failure mode the create-time check prevents.

uses the existing effectiveDealType machinery so a market->DDO switch
in the same PATCH is caught too. zero-valued price updates remain a
no-op (the request type uses pointers, so a real zero is harmless).
@parkan parkan enabled auto-merge (squash) April 23, 2026 12:02
@parkan parkan merged commit d10bb5e into data-preservation-programs:main Apr 23, 2026
1 check passed
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