Skip to content

fix: support millisecond quote stamps in simulations - #4880

Closed
6heLL66 wants to merge 1 commit into
cowprotocol:mainfrom
6heLL66:fix/metric-oracle-restamping
Closed

6heLL66 wants to merge 1 commit into
cowprotocol:mainfrom
6heLL66:fix/metric-oracle-restamping

Conversation

@6heLL66

@6heLL66 6heLL66 commented Sep 9, 2026 •

Copy link
Copy Markdown

Found this while testing Metric on staging. Titan quotes are for the next block, but we simulate them against the current block. Metric stores milliseconds in a different part of the storage word, so we miss it. This can cause FeedStalled() during simulation.
This adds the missing timestamp adjustment for Metric. The price data and existing seconds handling stay unchanged.
This format will be stable and will not change in the future.

Example:

This Metric word was captured from Titan for block 25925280:
0x5f3f82df171805f5e2f401018738ad0713148735fecc13140001a07bb2de3800
It contains the timestamp 1788781715000 ms. The simulation runs at the parent block, 25925279, whose timestamp is 1788781703000 ms — 12 seconds earlier.
The old code only recognizes seconds in the first four bytes. Metric stores milliseconds at offsets 25..31, so its timestamp is left unchanged and is in the future relative to the simulation.
With this fix, the word becomes:
0x5f3f82df171805f5e2f401018738ad0713148735fecc13140001a07bb2af5800

@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@6heLL66 6heLL66 changed the title fix: restamp Metric oracle quotes for simulation fix: support millisecond quote stamps in simulations Sep 9, 2026
@6heLL66
6heLL66 force-pushed the fix/metric-oracle-restamping branch 3 times, most recently from eb452eb to 1cb2f81 Compare September 9, 2026 13:15
@6heLL66
6heLL66 marked this pull request as ready for review September 9, 2026 14:13
@6heLL66
6heLL66 requested a review from a team as a code owner September 9, 2026 14:13
@6heLL66

6heLL66 commented Sep 9, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Sep 9, 2026

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

Thank you for your contribution!

Can you please rework the PR description to give a bit more background knowledge for people with less context on the matter (please don't AI generate).

Is the stamp format documented somewhere and expected to be stable or will other issues like this come up in the future?

Also, I feel a concrete example + test of the current limitation and proof of the fix is missing to give us confidence in accepting this change.

/// Newest stamp any venue quoted for. Only words carrying it belong to a
/// lane a maker is quoting for `block_number`.
stamp: Option<u32>,
millisecond_stamps: MillisecondStamps,

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.

Why do we need a new type here? Can we not capture stamp in the existing type (but use the Metric specific encoding for detection)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried using the same stamp, but a newer Metric quote could stop seconds quotes from being adjusted. Keeping them separate avoids changing the existing behaviour.

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.

I still don't understand why we need a full BTreeMap for the Metric lane, where for other lanes a single stamp seems to be fine? Could you explain this to me with an example (you may assume I'm not particularly familiar with propAMM's implementation details).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed the map and the extra per-word checks, I added it as an extra check against restamping stale quotes. It isn’t required for Metric’s format, and the existing seconds path doesn’t have it either.

@6heLL66
6heLL66 force-pushed the fix/metric-oracle-restamping branch from 1cb2f81 to 67f5c16 Compare September 15, 2026 15:46

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

I drafted #4911 that does't copy the stamp field and which gives more context to the reader on why this is needed. It passes the test you added, but would still ask @6heLL66 to verify this would also fix the issue and either update this PR or let me know so I can push the other one for review.

@6heLL66

6heLL66 commented Sep 16, 2026

Copy link
Copy Markdown
Author

Thanks @fleupold, tested #4911 with our captured Titan data and the oracle call now passes. Looks good, please go ahead with your PR.

@6heLL66 6heLL66 closed this Sep 17, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants