SIG-755: multi-asset oracle infrastructure#78
Conversation
- Per-market oracle config (feedId, feedDecimals, tickScale) appended to Market struct - createMarket signature accepts MarketOracleConfig, validates per-market config - _toSettlementTick extracted to OracleTickLib for single source of truth - Global oracle asset config storage-zombified (private + @Custom:oz-renamed-from) - setRedstoneConfig signature slimmed to protocol-level sample timing (2-param) - reinitializeV2 backfills all existing BTC markets atomically - New errors: InvalidOracleConfig, OracleConfigMissing(uint256) - Script + deploy config + TypeScript types updated to remove global feed keys - Safe MultiSend support added to PrepareSafeUpgrade.s.sol and scripts/ops/safe.ts
![signals-reviewer[bot]](https://avatars.githubusercontent.com/u/206232752?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Moves oracle configuration from protocol-wide globals to per-market values. Market struct gains three trailing fields (feedId, feedDecimals, tickScale), createMarket accepts a new MarketOracleConfig tuple, _toSettlementTick is extracted to OracleTickLib.sol, global feed storage is zombified with @custom:oz-renamed-from, reinitializeV2() backfills existing BTC markets, setRedstoneConfig is slimmed to 2 params, and PrepareSafeUpgrade switches to Safe MultiSend. 54 files changed, +1523 / -690.
Cross-PR Context
Sibling PRs (SIG-755)
- No sibling PRs found. The PR description states sibling repos (sdk, server, admin, subgraph, app) follow in separate issues against published ABIs. This is acceptable because v1-contract is the upstream source of truth and downstream repos consume the regenerated ABIs.
Impact Map Check
v1-subgraph:MarketCreatedevent gains 3 trailing fields,OracleConfigUpdatedevent args reduced from 4 to 2, newOracleConfigBackfilledevent. Subgraph update required but correctly deferred to follow-up issue.v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature change,setRedstoneConfigsignature change,redstoneFeedId()/redstoneFeedDecimals()getters removed,Marketstruct expanded. All deferred to follow-up issues.
Recent Review Patterns
- PR#75 (SIG-743, storage layout CI): flagged build-info caching and missing UUPS-discovery test. Neither pattern appears in this PR.
Issues
- 🟡 [Robustness]:
CreateMarket.s.solhard-codesbytes32("BTC"), feedDecimals: 8, tickScale: 1_000_000(line 177) and does not read oracle config from the market JSON template, while the template (market-template.json) has no oracle config fields either. (script/ops/CreateMarket.s.sol:177)Scenario: When operations needs to create a non-BTC market (the whole purpose of this multi-asset migration), they must remember to modify
CreateMarket.s.solsource code rather than updating a config file. This breaks the existing config-driven pattern where all market parameters come from JSON. If someone copies the template and fills in ETH market params but doesn't touch the script, the market gets BTC oracle config.
Evidence: Verifiedmarket-template.jsonhas nofeedId/feedDecimals/tickScalefields. VerifiedCreateMarket.s.solline 177 hard-codes BTC values. The PR description says "per-market setMarketOracleConfig setter is intentionally NOT added", butCreateMarket.s.solis the creation path, not the setter path. Addingoracle.feedId,oracle.feedDecimals,oracle.tickScaleto the JSON template and reading them in the script would maintain the config-driven pattern without requiring a setter.
Suggestions
- [Test Coverage]: No test verifies that
OracleTickLib.toSettlementTickhandlestickScale = 0(division by zero). The guard is in the callers (require(market.tickScale != 0)), but if a future caller forgets the guard, the EVM would revert with a generic panic rather than a meaningful error. (contracts/lib/OracleTickLib.sol:18)Scenario: A future function calls
OracleTickLib.toSettlementTickwithout checkingtickScale != 0. The EVM panics on division by zero (Panic(0x12)) instead of reverting withOracleConfigMissing. Currently all 3 callers have the guard, so this is defense-in-depth rather than a live bug.
Evidence: Verified all 3 call sites have thetickScale != 0guard before calling the library. The testOracleTickLib.t.soltests BTC scale parity, custom scale, and fuzz parity, but never tests the zero-tickScale path. Adding a test that verifies the EVM panic on tickScale=0 (or adding a require inside the library) would document the invariant.
Verdict
REQUEST_CHANGES: the CreateMarket.s.sol hard-coded oracle config issue is blocking. The ops script should read oracle configuration from the market JSON template to maintain the config-driven deployment pattern. The test coverage suggestion is also blocking per review rules.
There was a problem hiding this comment.
Summary
Moves oracle configuration from protocol-wide globals to per-market values. Market struct gains three trailing fields (feedId, feedDecimals, tickScale), createMarket accepts a new MarketOracleConfig tuple, _toSettlementTick is extracted to OracleTickLib.sol, global feed storage is zombified with @custom:oz-renamed-from, reinitializeV2() backfills existing BTC markets, setRedstoneConfig is slimmed to 2 params, and PrepareSafeUpgrade switches to Safe MultiSend. 54 files changed, +1523 / -690.
Cross-PR Context
Sibling PRs (SIG-755)
- No sibling PRs found. The PR description states sibling repos (sdk, server, admin, subgraph, app) follow in separate issues against published ABIs. This is acceptable because v1-contract is the upstream source of truth and downstream repos consume the regenerated ABIs.
Impact Map Check
v1-subgraph:MarketCreatedevent gains 3 trailing fields,OracleConfigUpdatedevent args reduced from 4 to 2, newOracleConfigBackfilledevent. Subgraph update required but correctly deferred to follow-up issue.v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature change,setRedstoneConfigsignature change,redstoneFeedId()/redstoneFeedDecimals()getters removed,Marketstruct expanded. All deferred to follow-up issues.
Recent Review Patterns
- PR#75 (SIG-743, storage layout CI): flagged build-info caching and missing UUPS-discovery test. Neither pattern appears in this PR.
Issues
- 🟡 [Robustness]:
CreateMarket.s.solhard-codesbytes32("BTC"), feedDecimals: 8, tickScale: 1_000_000(line 177) and does not read oracle config from the market JSON template, while the template (market-template.json) has no oracle config fields either. (script/ops/CreateMarket.s.sol:177)Scenario: When operations needs to create a non-BTC market (the whole purpose of this multi-asset migration), they must remember to modify
CreateMarket.s.solsource code rather than updating a config file. This breaks the existing config-driven pattern where all market parameters come from JSON. If someone copies the template and fills in ETH market params but doesn't touch the script, the market gets BTC oracle config.
Evidence: Verifiedmarket-template.jsonhas nofeedId/feedDecimals/tickScalefields. VerifiedCreateMarket.s.solline 177 hard-codes BTC values. The PR description says "per-market setMarketOracleConfig setter is intentionally NOT added", butCreateMarket.s.solis the creation path, not the setter path. Addingoracle.feedId,oracle.feedDecimals,oracle.tickScaleto the JSON template and reading them in the script would maintain the config-driven pattern without requiring a setter.
Suggestions
- [Test Coverage]: No test verifies that
OracleTickLib.toSettlementTickhandlestickScale = 0(division by zero). The guard is in the callers (require(market.tickScale != 0)), but if a future caller forgets the guard, the EVM would revert with a generic panic rather than a meaningful error. (contracts/lib/OracleTickLib.sol:18)Scenario: A future function calls
OracleTickLib.toSettlementTickwithout checkingtickScale != 0. The EVM panics on division by zero (Panic(0x12)) instead of reverting withOracleConfigMissing. Currently all 3 callers have the guard, so this is defense-in-depth rather than a live bug.
Evidence: Verified all 3 call sites have thetickScale != 0guard before calling the library. The testOracleTickLib.t.soltests BTC scale parity, custom scale, and fuzz parity, but never tests the zero-tickScale path. Adding a test that verifies the EVM panic on tickScale=0 (or adding a require inside the library) would document the invariant.
Verdict
REQUEST_CHANGES: the CreateMarket.s.sol hard-coded oracle config issue is blocking. The ops script should read oracle configuration from the market JSON template to maintain the config-driven deployment pattern. The test coverage suggestion is also blocking per review rules.
worjs
left a comment
worjs
left a comment
There was a problem hiding this comment.
Summary
Moves oracle configuration from protocol-wide globals to per-market values. Market struct gains three trailing fields (feedId, feedDecimals, tickScale), createMarket accepts a new MarketOracleConfig tuple, _toSettlementTick is extracted to OracleTickLib.sol, global feed storage is zombified with @custom:oz-renamed-from, reinitializeV2() backfills existing BTC markets, setRedstoneConfig is slimmed to 2 params, and PrepareSafeUpgrade switches to Safe MultiSend. 54 files changed, +1523 / -690.
Cross-PR Context
Sibling PRs (SIG-755)
- No sibling PRs found. The PR description states sibling repos (sdk, server, admin, subgraph, app) follow in separate issues against published ABIs. This is acceptable because v1-contract is the upstream source of truth and downstream repos consume the regenerated ABIs.
Impact Map Check
v1-subgraph:MarketCreatedevent gains 3 trailing fields,OracleConfigUpdatedevent args reduced from 4 to 2, newOracleConfigBackfilledevent. Subgraph update required but correctly deferred to follow-up issue.v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature change,setRedstoneConfigsignature change,redstoneFeedId()/redstoneFeedDecimals()getters removed,Marketstruct expanded. All deferred to follow-up issues.
Recent Review Patterns
- PR#75 (SIG-743, storage layout CI): flagged build-info caching and missing UUPS-discovery test. Neither pattern appears in this PR.
Issues
- 🟡 [Robustness]:
CreateMarket.s.solhard-codesbytes32("BTC"), feedDecimals: 8, tickScale: 1_000_000(line 177) and does not read oracle config from the market JSON template, while the template (market-template.json) has no oracle config fields either. (script/ops/CreateMarket.s.sol:177)Scenario: When operations needs to create a non-BTC market (the whole purpose of this multi-asset migration), they must remember to modify
CreateMarket.s.solsource code rather than updating a config file. This breaks the existing config-driven pattern where all market parameters come from JSON. If someone copies the template and fills in ETH market params but doesn't touch the script, the market gets BTC oracle config.
Evidence: Verifiedmarket-template.jsonhas nofeedId/feedDecimals/tickScalefields. VerifiedCreateMarket.s.solline 177 hard-codes BTC values. The PR description says "per-market setMarketOracleConfig setter is intentionally NOT added", butCreateMarket.s.solis the creation path, not the setter path. Addingoracle.feedId,oracle.feedDecimals,oracle.tickScaleto the JSON template and reading them in the script would maintain the config-driven pattern without requiring a setter.
Suggestions
- [Test Coverage]: No test verifies that
OracleTickLib.toSettlementTickhandlestickScale = 0(division by zero). The guard is in the callers (require(market.tickScale != 0)), but if a future caller forgets the guard, the EVM would revert with a generic panic rather than a meaningful error. (contracts/lib/OracleTickLib.sol:18)Scenario: A future function calls
OracleTickLib.toSettlementTickwithout checkingtickScale != 0. The EVM panics on division by zero (Panic(0x12)) instead of reverting withOracleConfigMissing. Currently all 3 callers have the guard, so this is defense-in-depth rather than a live bug.
Evidence: Verified all 3 call sites have thetickScale != 0guard before calling the library. The testOracleTickLib.t.soltests BTC scale parity, custom scale, and fuzz parity, but never tests the zero-tickScale path. Adding a test that verifies the EVM panic on tickScale=0 (or adding a require inside the library) would document the invariant.
Verdict
REQUEST_CHANGES: the CreateMarket.s.sol hard-coded oracle config issue is blocking. The ops script should read oracle configuration from the market JSON template to maintain the config-driven deployment pattern. The test coverage suggestion is also blocking per review rules.
Response to review (round 2)Thanks for the careful review. Round 2 applies the test coverage suggestion as Applied —
|
There was a problem hiding this comment.
Re-review Summary
1 new commit since last review (c18c5de: OracleTickLib zero-tickScale invariant test). 1 file changed.
Cross-PR Context
Sibling PRs (SIG-755)
- No sibling PRs found. v1-contract is the upstream source of truth; downstream repos (v1-subgraph, v1-sdk, v1-server, v1-admin, signals-app) will consume the regenerated ABIs in follow-up issues under the SIG-740 epic. This is acceptable.
Impact Map Check
v1-subgraph:MarketCreatedevent gains 3 trailing fields,OracleConfigUpdatedreduced from 4 to 2 args, newOracleConfigBackfilledevent. Deferred to sibling issue.v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature change,setRedstoneConfig4-to-2 arg reduction, removedredstoneFeedId()/redstoneFeedDecimals()getters,Marketstruct expanded. Deferred to sibling issues.
Previous Items
-
✅ [Resolved]: OracleTickLib zero-tickScale test coverage. Resolved by
c18c5deaddingtest_zeroTickScaleCausesEvmPanic()intest/foundry/unit/lib/OracleTickLib.t.sol. The test documents the EVM Panic(0x12) behavior whentickScale = 0, confirming the library relies on caller-side guards. The deliberate omission of a lib-internalrequireis well-reasoned: the library lacksmarketIdscope, so it cannot produce the more informativeOracleConfigMissing(marketId)error that the three production call sites emit. -
🔄 [Accepted]:
CreateMarket.s.solBTC oracle hard-code. The author's rebuttal meets the rigor standard with a 4-point argument backed by cross-repo evidence:- Verified BTC is hard-coded at every operational layer:
v1-server/src/config/configuration.ts:8(REDSTONE_DATA_FEED = 'BTC'),v1-integration/tests/helpers/redstone.js:4(DATA_FEED_ID = "BTC"),test/foundry/base/RedstoneHelper.sol:14(DATA_FEED_ID = bytes32("BTC")). No config-driven multi-asset pattern exists to break. - The "operator fills in ETH ticks, BTC oracle silently stamped" scenario is not silent: settlement tick would be visually absurd (BTC price ~100000 applied to ETH-range ticks), and subgraph-derived assetSymbol still reads "BTC". The reverse scenario (JSON-driven ETH oracle config with BTC-only server cron) produces a worse failure:
getOracleNumericValueFromTxMsg("ETH")reverts on the BTC Redstone payload, permanently blocking settlement. - Multi-asset enablement is a coordinated cross-stack change (8 items across 6+ repos), and premature template exposure creates a misleading interim state.
- The epic (SIG-740 Section 13 Open Decisions D1) explicitly defers multi-asset operations to post-audit.
- Verified BTC is hard-coded at every operational layer:
New Issues
No new issues found. Independent analysis of all 54 changed files confirms:
- Storage layout:
redstoneFeedId/redstoneFeedDecimalsslot retention via@custom:oz-renamed-fromis correct (types unchanged:bytes32anduint8). The 3 newMarketstruct fields (feedId,feedDecimals,tickScale) are appended afterdeltaEt, which is safe for mapping-stored structs. - reinitializeV2(): Loop bounds
1..nextMarketIdare correct (nextMarketIdis the next-to-assign value via++nextMarketId).numBins == 0skip correctly filters uninitialized market slots.reinitializer(2)prevents re-execution. Test coverage (test_reinitializeV2BackfillsExistingMarkets,test_reinitializeV2CannotRunTwice) is adequate. - OracleTickLib centralization: The library's
toSettlementTickis behaviorally identical to the old_toSettlementTickwhentickScale = 1_000_000(the BTC default). The fuzz testtestFuzz_btcScaleParityWithLegacyAlgorithmexhaustively validates this equivalence. - Oracle guards:
OracleConfigMissing(marketId)reverts are placed at all 3 entry points (submitSettlementSample,finalizePrimarySettlement,finalizeSecondarySettlement) before callingOracleTickLib. Test coverage for all 3 guards exists. abi.encodeWithSignatureselector: The string"createMarket(int256,int256,int256,uint64,uint64,uint64,uint32,uint256,address,address,(bytes32,uint8,uint64))"correctly representsMarketOracleConfigas its ABI tuple type.- Safe MultiSend encoding:
_encodeMultiSendTxusesuint8(0)(CALL) for both sub-transactions, which is correct because they target the coreProxy. The outer Safe transaction usesoperation=1(DELEGATECALL to MultiSend), and themultiSendAddressis cross-checked against the Safe SDK known network address at runtime. - simulateAndRevert decoder:
decodeSimulateAndRevertinsafe.tscorrectly handles both ABI-encoded and compact revert payload formats, with proper length checks. - build-redstone-submit-calldata.ts:
market.feedId ?? market[24]fallback is correct (feedId is the 25th field, index 24 in 0-based tuple decoding). - Fork test helper:
_alignCurrentBatchToDevForkStatecorrectly handles the dev-fork drift by scanning forward fromcurrentBatchIdand writing the aligned value viavm.store.FORK_SKIP_V2_UPGRADE=truefortest:fork:advisory:prodis correct since prod hasn't been upgraded yet.
Verdict
APPROVE: all previous items are Resolved (1) or Accepted (1). Zero Unresolved items. No new issues found.
There was a problem hiding this comment.
Re-review Summary
1 new commit since last review (c18c5de: OracleTickLib zero-tickScale invariant test). 1 file changed.
Cross-PR Context
Sibling PRs (SIG-755)
- No sibling PRs found. v1-contract is the upstream source of truth; downstream repos (v1-subgraph, v1-sdk, v1-server, v1-admin, signals-app) will consume the regenerated ABIs in follow-up issues under the SIG-740 epic. This is acceptable.
Impact Map Check
v1-subgraph:MarketCreatedevent gains 3 trailing fields,OracleConfigUpdatedreduced from 4 to 2 args, newOracleConfigBackfilledevent. Deferred to sibling issue.v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature change,setRedstoneConfig4-to-2 arg reduction, removedredstoneFeedId()/redstoneFeedDecimals()getters,Marketstruct expanded. Deferred to sibling issues.
Previous Items
-
✅ [Resolved]: OracleTickLib zero-tickScale test coverage. Resolved by
c18c5deaddingtest_zeroTickScaleCausesEvmPanic()intest/foundry/unit/lib/OracleTickLib.t.sol. The test documents the EVM Panic(0x12) behavior whentickScale = 0, confirming the library relies on caller-side guards. The deliberate omission of a lib-internalrequireis well-reasoned: the library lacksmarketIdscope, so it cannot produce the more informativeOracleConfigMissing(marketId)error that the three production call sites emit. -
🔄 [Accepted]:
CreateMarket.s.solBTC oracle hard-code. The author's rebuttal meets the rigor standard with a 4-point argument backed by cross-repo evidence:- Verified BTC is hard-coded at every operational layer:
v1-server/src/config/configuration.ts:8(REDSTONE_DATA_FEED = 'BTC'),v1-integration/tests/helpers/redstone.js:4(DATA_FEED_ID = "BTC"),test/foundry/base/RedstoneHelper.sol:14(DATA_FEED_ID = bytes32("BTC")). No config-driven multi-asset pattern exists to break. - The "operator fills in ETH ticks, BTC oracle silently stamped" scenario is not silent: settlement tick would be visually absurd (BTC price ~100000 applied to ETH-range ticks), and subgraph-derived assetSymbol still reads "BTC". The reverse scenario (JSON-driven ETH oracle config with BTC-only server cron) produces a worse failure:
getOracleNumericValueFromTxMsg("ETH")reverts on the BTC Redstone payload, permanently blocking settlement. - Multi-asset enablement is a coordinated cross-stack change (8 items across 6+ repos), and premature template exposure creates a misleading interim state.
- The epic (SIG-740 Section 13 Open Decisions D1) explicitly defers multi-asset operations to post-audit.
- Verified BTC is hard-coded at every operational layer:
New Issues
No new issues found. Independent analysis of all 54 changed files confirms:
- Storage layout:
redstoneFeedId/redstoneFeedDecimalsslot retention via@custom:oz-renamed-fromis correct (types unchanged:bytes32anduint8). The 3 newMarketstruct fields (feedId,feedDecimals,tickScale) are appended afterdeltaEt, which is safe for mapping-stored structs. - reinitializeV2(): Loop bounds
1..nextMarketIdare correct (nextMarketIdis the next-to-assign value via++nextMarketId).numBins == 0skip correctly filters uninitialized market slots.reinitializer(2)prevents re-execution. Test coverage adequate. - OracleTickLib centralization: Behaviorally identical to old
_toSettlementTickwhentickScale = 1_000_000. Fuzz test validates equivalence. - Oracle guards:
OracleConfigMissing(marketId)at all 3 entry points. Test coverage for all 3 exists. - ABI selector:
createMarket(...,(bytes32,uint8,uint64))correctly encodesMarketOracleConfigtuple. - Safe MultiSend:
_encodeMultiSendTxusesuint8(0)CALL for sub-txs (correct). Outer tx usesoperation=1DELEGATECALL.multiSendAddresscross-checked at runtime. - simulateAndRevert decoder: Handles both ABI-encoded and compact revert payloads correctly.
- build-redstone-submit-calldata.ts:
market[24]fallback is correct (feedId is 25th field, 0-indexed). - Fork test helper:
_alignCurrentBatchToDevForkStatehandles dev-fork drift correctly.FORK_SKIP_V2_UPGRADE=truefor advisory:prod is correct.
Verdict
APPROVE: all previous items are Resolved (1) or Accepted (1). Zero Unresolved items. No new issues found.
There was a problem hiding this comment.
Re-review Summary
1 new commit since last review (c18c5de: OracleTickLib zero-tickScale invariant test). 1 file changed.
Cross-PR Context
Sibling PRs (SIG-755)
- No sibling PRs found. v1-contract is the upstream source of truth; downstream repos (v1-subgraph, v1-sdk, v1-server, v1-admin, signals-app) will consume the regenerated ABIs in follow-up issues under the SIG-740 epic. This is acceptable.
Impact Map Check
v1-subgraph:MarketCreatedevent gains 3 trailing fields,OracleConfigUpdatedreduced from 4 to 2 args, newOracleConfigBackfilledevent. Deferred to sibling issue.v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature change,setRedstoneConfig4-to-2 arg reduction, removedredstoneFeedId()/redstoneFeedDecimals()getters,Marketstruct expanded. Deferred to sibling issues.
Previous Items
-
✅ [Resolved]: OracleTickLib zero-tickScale test coverage. Resolved by
c18c5deaddingtest_zeroTickScaleCausesEvmPanic()intest/foundry/unit/lib/OracleTickLib.t.sol. The test documents the EVM Panic(0x12) behavior whentickScale = 0, confirming the library relies on caller-side guards. The deliberate omission of a lib-internalrequireis well-reasoned: the library lacksmarketIdscope, so it cannot produce the more informativeOracleConfigMissing(marketId)error that the three production call sites emit. -
🔄 [Accepted]:
CreateMarket.s.solBTC oracle hard-code. The author's rebuttal meets the rigor standard with a 4-point argument backed by cross-repo evidence:- Verified BTC is hard-coded at every operational layer:
v1-server/src/config/configuration.ts:8(REDSTONE_DATA_FEED = 'BTC'),v1-integration/tests/helpers/redstone.js:4(DATA_FEED_ID = "BTC"),test/foundry/base/RedstoneHelper.sol:14(DATA_FEED_ID = bytes32("BTC")). No config-driven multi-asset pattern exists to break. - The "operator fills in ETH ticks, BTC oracle silently stamped" scenario is not silent: settlement tick would be visually absurd (BTC price ~100000 applied to ETH-range ticks), and subgraph-derived assetSymbol still reads "BTC". The reverse scenario (JSON-driven ETH oracle config with BTC-only server cron) produces a worse failure:
getOracleNumericValueFromTxMsg("ETH")reverts on the BTC Redstone payload, permanently blocking settlement. - Multi-asset enablement is a coordinated cross-stack change (8 items across 6+ repos), and premature template exposure creates a misleading interim state.
- The epic (SIG-740 Section 13 Open Decisions D1) explicitly defers multi-asset operations to post-audit.
- Verified BTC is hard-coded at every operational layer:
New Issues
No new issues found. Independent analysis of all 54 changed files confirms:
- Storage layout:
redstoneFeedId/redstoneFeedDecimalsslot retention via@custom:oz-renamed-fromis correct (types unchanged:bytes32anduint8). The 3 newMarketstruct fields (feedId,feedDecimals,tickScale) are appended afterdeltaEt, which is safe for mapping-stored structs. - reinitializeV2(): Loop bounds
1..nextMarketIdare correct (nextMarketIdis the next-to-assign value via++nextMarketId).numBins == 0skip correctly filters uninitialized market slots.reinitializer(2)prevents re-execution. Test coverage adequate. - OracleTickLib centralization: Behaviorally identical to old
_toSettlementTickwhentickScale = 1_000_000. Fuzz test validates equivalence. - Oracle guards:
OracleConfigMissing(marketId)at all 3 entry points. Test coverage for all 3 exists. - ABI selector:
createMarket(...,(bytes32,uint8,uint64))correctly encodesMarketOracleConfigtuple. - Safe MultiSend:
_encodeMultiSendTxusesuint8(0)CALL for sub-txs (correct). Outer tx usesoperation=1DELEGATECALL.multiSendAddresscross-checked at runtime. - simulateAndRevert decoder: Handles both ABI-encoded and compact revert payloads correctly.
- build-redstone-submit-calldata.ts:
market[24]fallback is correct (feedId is 25th field, 0-indexed). - Fork test helper:
_alignCurrentBatchToDevForkStatehandles dev-fork drift correctly.FORK_SKIP_V2_UPGRADE=truefor advisory:prod is correct.
Verdict
APPROVE: all previous items are Resolved (1) or Accepted (1). Zero Unresolved items. No new issues found.
1 participant
Context
Jira: SIG-755 (parent epic SIG-740 multi-asset oracle migration).
Moves oracle configuration from protocol-wide globals to per-market values so future non-BTC markets can be added without redeploying. This PR is the contract track; sibling repos (sdk, server, admin, subgraph, app) follow in separate issues.
Decisions
Marketstruct gains three trailing fieldsfeedId,feedDecimals,tickScale(append-only).MarketOracleConfigis a new tuple introduced besideMarketinISignalsCoreand is the 11thcreateMarketargument._toSettlementTickis extracted tocontracts/lib/OracleTickLib.solas the single source of truth. The hard-coded1_000_000divisor in the old copies is replaced withmarket.tickScale.redstoneFeedId/redstoneFeedDecimalsstorage is preserved at the same slots, renamed toprivate __deprecated_*with@custom:oz-renamed-from. The public getters disappear as a consequence.setRedstoneConfigis retained but slimmed to(uint64 maxSampleDistance, uint64 futureTolerance). Per-asset values now live on each market.reinitializeV2()(reinitializer(2) onlyOwner) backfills existing markets with hard-coded BTC defaults (feedId="BTC",feedDecimals=8,tickScale=1_000_000) and emitsOracleConfigBackfilled(marketCount). Markets withnumBins == 0are skipped.setMarketOracleConfigsetter is intentionally NOT added in this PR — deferred to a future operations issue.PrepareSafeUpgradeswitches the upgrade plan from a singleupgradeToAndCallto a SafeMultiSend(operation=1DelegateCall) bundlingupgradeToAndCall(newImpl, reinitializeV2())followed bysetModules(...). ThemultiSendAddressis loaded fromscript/config/deploy-<env>.jsonand cross-checked against the Safe SDK's known network address.MODULESforPrepareSafeUpgradechanges fromtrade,lifecycle,vaulttolifecycle,oracle, with a hard requirement that both be present for the SIG-755 cutover.Impact
ABI surface changes that downstream repos consume:
v1-subgraph:MarketCreatedevent gains three trailing fields (bytes32 feedId,uint8 feedDecimals,uint64 tickScale). New eventOracleConfigBackfilled(uint256 marketCount).OracleConfigUpdatedevent arg list is reduced from 4 to 2 (dropsfeedId,feedDecimals).v1-sdk/v1-server/v1-admin/signals-app:createMarketsignature gains an 11th argumentMarketOracleConfig.setRedstoneConfigsignature dropsfeedIdandfeedDecimals(now 2-arg). TheredstoneFeedId()andredstoneFeedDecimals()view getters are removed — callers must readgetMarket(marketId).feedId / .feedDecimalsinstead.Marketstruct gains 3 trailing fields, shifting any positional ABI decoders. New errorsInvalidOracleConfig,OracleConfigMissing(uint256).abis/*.jsonand storage layouts instorage-snapshots/*.jsonare the source of truth for inline-ABI updates in sibling repos.No sibling PRs are open yet (search returned none). Sibling updates land in follow-up issues against this PR's published ABIs.
Risk Areas
redstoneFeedId/redstoneFeedDecimalsslot retention via@custom:oz-renamed-from, newMarkettrailing fields. Storage diff CI gate applies.upgradeToAndCalltoMultiSendDelegateCall.simulateTxnow branches onoperationand usessimulateAndRevertfor delegatecalls; new bespoke decoderdecodeSimulateAndReverthandles Safe's compact revert payload.reinitializeV2(): iterates1..nextMarketIdwriting to existing market storage. ThenumBins == 0skip is the only filter; if any non-BTC fixture market exists it will be silently mislabeled as BTC. Production state is BTC-only at this revision._toSettlementTickare replaced. Behavioral parity fortickScale=1_000_000is the key invariant.OracleConfigMissing(marketId)reverts atsubmitSettlementSample, primary settlement finalize, andfinalizeSecondarySettlementwhenfeedId == bytes32(0)ortickScale == 0. Markets that miss the backfill cannot settle.test/foundry/fork/base/ForkBaseTest.sol): bypassescurrentBatchIdand_batchAggregationsslot writes directly to repair a live dev-fork drift where processed-batch flags are ahead ofcurrentBatchId. Scope is fork test setup only.abi.encodeWithSignatureselector inSignalsCore.createMarketdelegation: the string-encoded signature"createMarket(...,(bytes32,uint8,uint64))"must match the actual lifecycle module function exactly for the delegatecall selector to resolve.Findings
PrepareSafeUpgradepreviously hard-coded a per-tx layout insafe-upgrade-copy/files and a 6-step Safe UI flow. Filenames and ordering have changed; any external runbook referencing the old01-to.txt … 06-calldata-upgradeToAndCall.txtlayout needs an update outside this repo.