feat[oz-retainer-07]: add reversible pre-proposal pause and reward recovery - #56
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reinis-FRP
left a comment
There was a problem hiding this comment.
Approved. The implementation preserves the intended requester/request-manager funding and refund boundaries and keeps OOReporter synchronized for re-requests. The targeted ABI decoding and guarded unchecked arithmetic are acceptable maintainability compromises to stay within the tight EIP-170 bytecode limits.
Reinis Codex coding agent 🤖
| /** | ||
| * @notice Updates the reward associated with a price request. | ||
| * @dev Only callable while the request is in State.Requested (before any proposal). Increases are pulled from the | ||
| * caller, while decreases are refunded to the requester and may be deferred if the transfer fails. |
There was a problem hiding this comment.
why make distinction here as setReward always binds request with msg.sender, so only the original requester can call this?
There was a problem hiding this comment.
Correct—on setReward, msg.sender is both the caller and requester. The distinction came from the shared _setReward path, where requestManagerSetReward has a different payer and refund recipient. I tightened the implementation and interface NatSpec in c2fc9c5 to state directly that the requester funds increases and receives decreases.
chrismaree
left a comment
There was a problem hiding this comment.
FRO-106’s current acceptance criteria say direct request-manager reward changes “must not leave OOReporter.RequestData.reward stale” and that OOReporter keeps its cached reward synchronized with MOOv2. This head intentionally permits temporary staleness after requestManagerSetReward; the test asserts it, and the README documents resynchronization only on a later initializer update or dispute callback.
The execution paths appear safe because reporter-side updates read the live oracle reward and the dispute callback resynchronizes before an automatic re-request. The public getRequest() cache is nevertheless non-authoritative during that interval, so the implementation does not satisfy the issue as written.
Please either add an explicit synchronization mechanism, or get product/security acceptance for temporary cache staleness and update FRO-106’s requirements and docs to make that relaxation unambiguous. This is a scope/contract decision rather than a small local patch.
Sent from Chris Codex Agent 🤖
|
The temporary cache lag after a direct request-manager update is now accepted and documented behavior. FRO-106’s limitations, acceptance criteria, and focused-test wording now make Managed OO authoritative during that interval. Commit c2fc9c5 also clarifies the README: @chrismaree please re-review the updated scope and documentation. |
Feature
FRO-106 adds a reversible pre-proposal pause and reward-recovery flow for active Managed OO requests. This is an additional Retainer 07 feature rather than an audit finding.
References: FRO-106 · audited scope tag
Resolution
Requested.address(0)restores the default whitelist, whileDisabledAddressWhitelistremains permissionless.ManagedOptimisticOracleV2deployable under EIP-170.Validation
forge test --match-contract 'ManagedOptimisticOracleV2Test|DeferredPayoutTest'— 73 tests passedcd pm-v2-oo-reporter && forge test --match-path test/OOReporter.t.sol— 45 tests passedforge build --sizes—ManagedOptimisticOracleV224,319 B (257 B margin)cd pm-v2-oo-reporter && forge build --sizes --optimize false—OOReporter24,490 B (86 B margin)cd pm-v2-oo-reporter && forge fmt --checkgit diff --check