Conversation
|
0xKitsune
left a comment
There was a problem hiding this comment.
Lgtm, just a few minor nits.
📊 Tempo Precompiles CoverageprecompilesCoverage: 5681/7745 lines (73.35%) File details
contractsCoverage: 1/217 lines (0.46%) File details
Total: 5682/7962 lines (71.36%) |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — No actionable findings.
This PR implements TIP-1056 (keep order IDs across flips on T5+). All six worker iterations across two workers (domain audit, ripple analysis, invariant breaking) returned NO_FINDING. Linked-list and tick-bitmap integrity hold for same-tick "locked book" flips, escrow/refund rounding is symmetric (Up/Up for bids, exact for asks), atomicity is enforced by an inner CheckpointGuard, system errors propagate on T1A+, and the keep_record flag prevents orphaned records on flip failure. T5 is dormant on all chains, so the new path is not yet active in production.
Reviewer Callouts
- ⚡ TIP-1055 storage version upgrade (future work): TIP-1056 §Rewritten state mandates that when TIP-1055 is active, a flip rewriting a v0 order MUST upgrade it to v1 and clear deprecated slot 0. TIP-1055 is not implemented in this PR;
flip_in_placecurrently writes the whole record viaStorable::write. When TIP-1055 lands,flip_in_placewill need version-upgrade logic. - ⚡ "Only changed slots written" gas-MUST: Spec §74 says the implementation MUST write only order-record slots whose post-flip contents differ.
commit_order_to_bookcurrently writes the entireOrder. Spec compliance / gas concern, not a security issue, and not enforced by any invariant test. - ⚡ Off-chain indexer / UI breaking change: External consumers tracking active liquidity via
OrderPlaced+OrderCancelledalone will misclassify flipped orders on T5+. They MUST subscribe toOrderFlippedto maintain correct state. Documented in TIP-1056 §Compatibility but worth surfacing in release notes. - ⚡
cancel(orderId)semantics on T5+: After a flip,cancel(id)refunds the current (post-flip) escrow token, which differs from the pre-flip token. Net economic value to the maker is preserved, so this is not a fund-loss path, but UIs/contracts that pre-signed cancellations expecting a specific token must adapt. - ⚡
tips/verify/test/StablecoinDEX.t.sol::test_FlipOrderExecution(line 363): Hard-codes pre-T5 expectations (OrderPlaced(2,...),nextOrderId == 3). Will silently break whenfoundry.toml's default fork moves to T5+ unless updated to expectOrderFlippedand unchangednextOrderId. - ⚡ Quote functions vs. flip semantics:
quote_exact_in/outwalk one side of the book and don't model what happens if a swap path crosses freshly-flipped liquidity at a same-tick locked book. Pre-existing limitation (cf. SIGP-TMPO3-33), unchanged by this PR. - ⚡
HashMapStorageProvidercheckpoint behavior in tests:test_flip_in_place_failure_no_orphan_t5relies oncheckpoint_revertrolling back emitted events under the test storage provider. Worth a quick spot-check that the test-only provider really does revert logs.
Linear: https://linear.app/tempoxyz/issue/CHAIN-1140/tip-1056-keep-order-ids-across-flips