Skip to content

Support for validFrom app-data field - #4672

Open
AryanGodara wants to merge 8 commits into
mainfrom
aryan/order-valid-from
Open

Support for validFrom app-data field#4672
AryanGodara wants to merge 8 commits into
mainfrom
aryan/order-valid-from

Conversation

@AryanGodara

@AryanGodara AryanGodara commented Jul 27, 2026

Copy link
Copy Markdown
Member

Description

Add validFrom (app-data, unix seconds). An order enters the batch auction only once now >= validFrom. NULL/absent = eligible immediately (unchanged for existing orders). Groundwork for fast-path.

Changes

  • Nullable orders.valid_from column (V115) + partial index (V116), no backfill.
  • Gated in the solvable-orders SQL code.
  • Onchain orders backfill valid_from from app-data inside insert_order_hooks (reusing the fetch it already does).
  • Rejects validFrom >= validTo; a past validFrom is fine.

Testing

  • DB: full-build gating + both incremental branches + on-chain backfill.
  • Unit: validFrom >= validTo rejection.
  • e2e: a regular EIP-712 order and an on-chain ethflow order are both held out of the auction until validFrom, then settle.

Note:

Overlaps #4668, compared the two, fixed e2e tests accordingly, and removed some redundant unit tests, and focused the PR

@AryanGodara AryanGodara self-assigned this Jul 27, 2026
@github-actions

Copy link
Copy Markdown

Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).

  • If creating new tables, update the tables list.
  • When adding a new index, consider using CREATE INDEX CONCURRENTLY for tables involved in the critical execution path.
  • For breaking changes, remember that during rollout k8s starts the new autopilot, runs the Flyway migration, and only then shuts down the old pod. That overlap means the previous version can still be processing requests on the migrated schema, so make it compatible first and ship the breaking DB change in the following release.

Caused by:

@github-actions

Copy link
Copy Markdown

Reminder: Please consider backward compatibility when modifying the API specification.
If breaking changes are unavoidable, ensure:

  • You explicitly pointed out breaking changes.
  • You communicate the changes to affected teams (at least Frontend team and SAFE team).
  • You provide proper versioning and migration mechanisms.

Caused by:

Comment thread crates/autopilot/src/database/onchain_order_events/mod.rs Outdated
@AryanGodara AryanGodara changed the title [possible duplicate, for reference] Support the validFrom app-data field Support for validFrom app-data field Jul 28, 2026
@AryanGodara
AryanGodara marked this pull request as ready for review July 28, 2026 07:29
@AryanGodara
AryanGodara requested a review from a team as a code owner July 28, 2026 07:30
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AryanGodara's task in 5m 0s —— View job


PR Review: Support for validFrom app-data field

I reviewed the full diff against origin/main. Overall this is a clean, well-tested change — the gating logic is correct and the tricky "pending→valid is not a DB write" incremental case is handled properly.

Things I verified as correct (no action needed):

  • No gaps in the incremental refresh. Branch 2 of open_orders_by_time_or_uids re-selects valid_from ∈ (after_timestamp, now], and solvable_orders.rs:447 pushes the checkpoint back 60s each cycle, giving a generous overlap so a validFrom crossing is never missed. On restart the cache is empty and the fully-gated all_solvable_orders path runs, so no gap there either.
  • INSERT bind sequence ($21=true_valid_to, $22=valid_from) matches the column list — 22 columns, 22 binds.
  • jit_orders SELECT correctly adds NULL AS valid_from to stay aligned with FullOrder (by-name FromRow).
  • Backfill orderinginsert_order_hooks mutates orders before insert_orders_and_ignore_conflicts, so the backfilled value is persisted.
  • Backward compat — nullable column + partial index, no backfill, skip_serializing_if on the new API field. CREATE INDEX CONCURRENTLY follows existing repo precedent (V102/V112).

Two findings posted inline:

  1. onchain_order_events/mod.rs — the valid_from backfill is an implicit, name-hidden side effect of insert_order_hooks with an invisible ordering dependency on the subsequent insert. Suggest extracting it or documenting the dependency.
  2. order_validation.rs — the validFrom >= validTo guard only covers API orders; on-chain/ethflow orders bypass it and would be silently held out until expiry. Worth a deliberate decision + comment.

Neither is blocking. Nice work on the test coverage (DB gating + both incremental branches + on-chain backfill + e2e for both order types).

Comment thread crates/autopilot/src/database/onchain_order_events/mod.rs Outdated
Comment thread crates/shared/src/order_validation.rs
Comment thread crates/database/src/orders.rs Outdated
Comment thread crates/database/src/orders.rs
Comment thread crates/database/src/orders.rs

@MartinquaXD MartinquaXD 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.

Also I'm a bit hesitant about the runtime of the new query. We are already adding an index for the new column but we should probably do a test run on the prod DB before merging this PR.

Comment on lines +878 to +884
WHERE (
(o.creation_timestamp > $1 OR o.cancellation_timestamp > $1 OR o.uid = ANY($2))
AND (o.valid_from IS NULL OR o.valid_from <= $3)
)
OR (o.valid_from IS NOT NULL
AND o.valid_from > EXTRACT(EPOCH FROM $1)::bigint
AND o.valid_from <= $3)

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.

Actually after thinking more about it it's not clear to me why we even split block based change detection from time based change detection. It seems like that could become 1 giant DB query. That way we save on roundtrips and possible help the query planner optimize better. 🤔
This would mean quite a change so it's out of scope for this PR.

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