feat(proxy-oracle-governance): reflexive vs. target-function-call operations (ENG-516)#527
Open
peer2f00l wants to merge 1 commit into
Conversation
…rations (ENG-516) Split `Operation` into `Reflexive(ReflexiveOperation)` and a generic `TargetFunctionCall(FunctionCall)`, collapse `execute_proposal` to reflexive arms plus one dispatched call, and make per-method TTL *and* role table-driven via `GovernancePolicy` (independent reflexive timelocks, a conservative `default_target` ceiling, and a `method_policies` whitelist). Adding a new target op now needs a `SetMethodPolicy` proposal, not a governance-contract upgrade. - Resolution: a listed method uses its override; any unlisted method falls back to `default_target` (long TTL, Admin), and every override is held `ttl <= default_target.ttl` on write — authoritative at execute time. - Typed builders move client-side: the manager CLI keeps its subcommands and builds the generic form via the shared `LegacyOperation` mapping, which also powers legacy-JSON acceptance (`#[serde(try_from)]`) and the borsh migration. - In-place v0->v1 migration (v1 was never released) seeds the policy from the old flat TTL table and rewrites pending proposal bodies to the generic form. - Retire the dead kernel `TtlConfig<Kind>` trait; bump governance `event_version` to 3.0.0 with a `method` field; swap gateway `getOperationTtl` for `getGovernancePolicy` and regenerate METHODS.md. The old create-time payload checks (empty proxy, over-long breaker history) are intentionally not reproduced in the now-opaque generic form; enforcing them at the proxy-oracle contract is tracked by ENG-520. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
📝 WalkthroughSummary
Human review focus
WalkthroughThe PR replaces TTL-per-operation governance with a ChangesGovernance policy and operation wire
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Gateway
participant GovernanceContract
participant ProxyOracle
CLI->>Gateway: submit policy or proposal request
Gateway->>GovernanceContract: create proposal or read policy
GovernanceContract->>GovernanceContract: resolve policy and timelock
GovernanceContract->>ProxyOracle: execute target admin_* call
ProxyOracle-->>GovernanceContract: receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640f0be3a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Operation::Reflexive( | ||
| ReflexiveOperation::SetRole { .. } | ReflexiveOperation::SelfUpgrade { .. }, | ||
| ) => Role::Admin, | ||
| Operation::Reflexive(_) => Role::ProxyConfigurationManager, |
There was a problem hiding this comment.
With only ProxyConfigurationManager, a caller can create and execute SetMethodPolicy for any method because all policy edits use this role. In the seeded policy, that caller can set admin_upgrade to { ttl: 0, role: ProxyConfigurationManager } and then immediately create/execute a proxy-oracle upgrade without holding Admin, bypassing the Admin-only upgrade boundary; policy edits that downgrade an existing Admin-gated method should require the method's current role or Admin.
Useful? React with 👍 / 👎.
| std::cmp::max(set_action_ttl, target_ttl) | ||
| } | ||
| _ => ttls.get(self.kind()), | ||
| Operation::Reflexive(reflexive) => policy.reflexive_ttls.get(reflexive.kind()), |
There was a problem hiding this comment.
When set_policy is shorter than self_upgrade (or another protected bucket), SetReflexiveTtl { kind: SelfUpgrade, ttl: 0 } matures using only the short SetPolicy TTL here. Once it executes, the next SelfUpgrade proposal can use the shortened TTL, so the configured self-upgrade delay is not an effective ceiling; the minimum TTL for SetReflexiveTtl should include the current TTL of the bucket being changed before it can be shortened.
Useful? React with 👍 / 👎.
Comment on lines
+277
to
+280
| Operation::Reflexive(ReflexiveOperation::SetMethodPolicy { | ||
| method: method.to_owned(), | ||
| policy: Some(MethodPolicy { ttl, role }), | ||
| }) |
There was a problem hiding this comment.
Migrating a pending legacy SetActionTtl target edit directly into SetMethodPolicy can make the proposal fail under the new ttl <= default_target.ttl invariant. For example, if the old policy had all target TTLs at 1 day and a pending SetActionTtl(SetProxy, 7 days), this maps to a method policy above the migrated default and execute_proposal rejects it even though the legacy proposal would have succeeded; the migration needs to adjust the default target policy (or otherwise preserve the old TTL-edit semantics) along with the method override.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/manager/src/commands/proxy_oracle/governance/create_proposal.rs (1)
130-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the legacy-route vs. direct-construction arms.
into_operationbinds most arms tolegacy: LegacyOperationthen converts once at the end, but three reflexive-policy arms bypass this via earlyreturn Ok(...). It compiles (never-type coercion) but the mixed control flow is harder to follow at a glance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/manager/src/commands/proxy_oracle/governance/create_proposal.rs` around lines 130 - 243, Refactor into_operation so the legacy-routed arms and direct reflexive-policy construction are separated instead of mixing early returns into the LegacyOperation match. Keep the existing LegacyOperation conversion for target and upgrade commands, and handle SetReflexiveTtl, SetTargetDefault, and SetMethodPolicy through a clearly separate direct-construction path with unchanged behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/proxy-oracle/README.md`:
- Line 52: Update the role-policy introduction in the README to say “The roles
required by a method come from its resolved policy.” Keep the seeded-default
role mappings and governance behavior unchanged.
- Around line 38-40: Update the GovernancePolicy description around
method_policies and the new target-method guidance to state that SetMethodPolicy
is only required when granting a method a non-default TTL or role; unlisted
methods remain dispatchable through default_target.
In `@tools/manager/src/commands/proxy_oracle/governance/create_proposal.rs`:
- Around line 355-369: Update SetMethodPolicyArgs so --ttl is required for
non-reset set-method-policy calls, mirroring the role argument’s
required_unless_present = "reset" gating; remove its zero-duration default so
omitted TTL fails validation, while reset operations may continue omitting it.
---
Outside diff comments:
In `@tools/manager/src/commands/proxy_oracle/governance/create_proposal.rs`:
- Around line 130-243: Refactor into_operation so the legacy-routed arms and
direct reflexive-policy construction are separated instead of mixing early
returns into the LegacyOperation match. Keep the existing LegacyOperation
conversion for target and upgrade commands, and handle SetReflexiveTtl,
SetTargetDefault, and SetMethodPolicy through a clearly separate
direct-construction path with unchanged behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec73f8b0-005c-4ba6-ab1a-2be941482b77
📒 Files selected for processing (29)
contract/proxy-oracle/README.mdcontract/proxy-oracle/governance-kernel/src/lib.rscontract/proxy-oracle/near/governance-common/src/interface.rscontract/proxy-oracle/near/governance-common/src/legacy.rscontract/proxy-oracle/near/governance-common/src/lib.rscontract/proxy-oracle/near/governance-common/src/tests.rscontract/proxy-oracle/near/governance-contract/src/lib.rscontract/proxy-oracle/near/governance-contract/src/state.rscontract/proxy-oracle/near/governance-contract/src/state/legacy.rscontract/proxy-oracle/near/governance-contract/src/state/migration.rscontract/proxy-oracle/near/governance-contract/src/state/mod.rscontract/proxy-oracle/near/governance-contract/src/tests.rscontract/proxy-oracle/near/governance-contract/tests/upgrade_ordering.rscontract/proxy-oracle/soroban/governance-common/src/lib.rsgateway/METHODS.mdgateway/core/src/client/proxy_governance.rsgateway/methods-dispatch/src/proxy_oracle_governance_impl.rsgateway/methods-spec/src/lib.rsgateway/methods-spec/src/proxy_oracle_governance.rsgateway/testing/src/sandbox.rsservice/gateway/src/rpc/tests/proxy_oracle_tests.rstools/manager/src/commands/proxy_oracle/governance/create.rstools/manager/src/commands/proxy_oracle/governance/create_proposal.rstools/manager/src/commands/proxy_oracle/governance/get_governance_policy.rstools/manager/src/commands/proxy_oracle/governance/get_operation_ttl.rstools/manager/src/commands/proxy_oracle/governance/mod.rstools/manager/src/dispatch/mod.rstools/manager/src/tests/deploy_script.rstools/manager/src/tests/proxy_oracle.rs
💤 Files with no reviewable changes (3)
- tools/manager/src/commands/proxy_oracle/governance/get_operation_ttl.rs
- contract/proxy-oracle/soroban/governance-common/src/lib.rs
- contract/proxy-oracle/near/governance-contract/src/state.rs
Comment on lines
+38
to
+40
| An `Operation` is either **reflexive** — mutating the governance contract's own state (`SetReflexiveTtl`, `SetTargetDefault`, `SetMethodPolicy`, `SetRole`, `SelfUpgrade`) — or a **`TargetFunctionCall`**: a generic `(method_name, args, attached_deposit, gas)` call dispatched to the governed proxy oracle. Every proxy/circuit-breaker action is a target call to the matching `admin_*` method; adding a new one needs **no governance-contract upgrade**, only a `SetMethodPolicy` entry. Governance treats the target payload as opaque bytes — semantic validation of the payload happens at the proxy oracle's `admin_*` method (and so fails only at execution, after the timelock), not at proposal-create time. The manager CLI keeps typed subcommands (`set-proxy`, `add-circuit-breaker`, …) that build the generic form client-side and still catch malformed input locally. | ||
|
|
||
| Per-method timelock and role come from a table-driven `GovernancePolicy`: independent reflexive timelocks, a conservative `default_target` (a long TTL and `Role::Admin`), and a `method_policies` whitelist of `{ ttl, role }` overrides for known cheap/low-privilege methods. Resolution: a listed method uses its override; any unlisted method — including one introduced by a future target upgrade — falls back to `default_target`, so it can never buy a shorter timelock or a lower role than the default. Every override's TTL is held `<= default_target.ttl` on every write. Because `SelfUpgrade` can replace the contract with arbitrary code, its reflexive timelock is the effective ceiling on all others and should be deployed as the longest lock. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify when SetMethodPolicy is required.
Because unlisted methods fall back to default_target, a new target method does not require a policy entry to be dispatchable. Clarify that SetMethodPolicy is needed to grant a non-default TTL or role.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/proxy-oracle/README.md` around lines 38 - 40, Update the
GovernancePolicy description around method_policies and the new target-method
guidance to state that SetMethodPolicy is only required when granting a method a
non-default TTL or role; unlisted methods remain dispatchable through
default_target.
| `get_proxy_circuit_breaker_set` exposes both `accepted_history` and `observed_history`. Accepted history is the rule baseline and only records non-blocking evaluations. Observed history records valid sampled prices even while the set is tripped or manually blocked, and should be treated as recovery/audit data until governance explicitly seeds from it. | ||
|
|
||
| Manual trip/untrip is available through governance operation `SetManualTrip { id, is_manually_tripped, metadata }` for offline incident response and requires `Role::ManualTripper`. Circuit-breaker lifecycle operations `Rearm` and `SetEnforced` require `Role::CircuitBreakerOperator`. Proxy definitions, circuit-breaker configuration, breaker add/remove, and TTL policy changes require `Role::ProxyConfigurationManager`. Governance roles are multi-role memberships managed with targeted `SetRole { account_id, role, set }` operations: `set: true` grants the named role and `set: false` revokes only that named role. `Role::Admin` is the global governance superuser role and may act on any proposal; removing the final `Role::Admin` membership is rejected. | ||
| The roles a method requires come from its resolved policy. Under the seeded defaults, manual trip/untrip (`admin_set_manual_trip`) requires `Role::ManualTripper`; circuit-breaker lifecycle methods (`admin_rearm`, `admin_set_enforced`) require `Role::CircuitBreakerOperator`; proxy definitions and circuit-breaker configuration/add/remove require `Role::ProxyConfigurationManager`. Reflexive policy edits require `Role::ProxyConfigurationManager`; `SetRole` and `SelfUpgrade` require `Role::Admin`. Governance roles are multi-role memberships managed with targeted `SetRole { account_id, role, set }` operations: `set: true` grants the named role and `set: false` revokes only that named role. `Role::Admin` is the global governance superuser role and may act on any proposal; removing the final `Role::Admin` membership is rejected. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Improve the role-policy wording.
Rewrite this as: “The roles required by a method come from its resolved policy.”
🧰 Tools
🪛 LanguageTool
[style] ~52-~52: The double modal “requires come” is nonstandard (only accepted in certain dialects). Consider “to be come”.
Context: ...s from it. The roles a method requires come from its resolved policy. Under the see...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/proxy-oracle/README.md` at line 52, Update the role-policy
introduction in the README to say “The roles required by a method come from its
resolved policy.” Keep the seeded-default role mappings and governance behavior
unchanged.
Source: Linters/SAST tools
Comment on lines
+355
to
369
| #[derive(Args, Debug)] | ||
| pub struct SetMethodPolicyArgs { | ||
| /// Target method name (e.g. `admin_set_proxy`). | ||
| #[arg(long, value_name = "NAME")] | ||
| method: String, | ||
| /// Timelock for this method (ignored with `--reset`). Must be `<=` the target default. | ||
| #[arg(long, value_name = "DURATION", value_parser = parse_duration, default_value = "0ns")] | ||
| ttl: Nanoseconds, | ||
| /// Role required to invoke this method. Required unless `--reset`. | ||
| #[arg(long, value_enum, required_unless_present = "reset")] | ||
| role: Option<Role>, | ||
| /// Remove the override, resetting the method to the target default. | ||
| #[arg(long, conflicts_with = "role")] | ||
| reset: bool, | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
--ttl silently defaults to zero timelock on real (non-reset) set-method-policy calls.
ttl has default_value = "0ns" but, unlike role, is not gated by required_unless_present = "reset". An operator setting a real method policy who forgets --ttl gets a zero-timelock policy for that method with no error — silently defeating the governance timelock this PR is built around.
🔒 Proposed fix: mirror the `role` gating pattern
pub struct SetMethodPolicyArgs {
/// Target method name (e.g. `admin_set_proxy`).
#[arg(long, value_name = "NAME")]
method: String,
- /// Timelock for this method (ignored with `--reset`). Must be `<=` the target default.
- #[arg(long, value_name = "DURATION", value_parser = parse_duration, default_value = "0ns")]
- ttl: Nanoseconds,
+ /// Timelock for this method (ignored with `--reset`). Must be `<=` the target default.
+ /// Required unless `--reset`.
+ #[arg(long, value_name = "DURATION", value_parser = parse_duration, required_unless_present = "reset")]
+ ttl: Option<Nanoseconds>,
/// Role required to invoke this method. Required unless `--reset`.
#[arg(long, value_enum, required_unless_present = "reset")]
role: Option<Role>,
/// Remove the override, resetting the method to the target default.
- #[arg(long, conflicts_with = "role")]
+ #[arg(long, conflicts_with_all = ["role", "ttl"])]
reset: bool,
}And in into_operation's SetMethodPolicy arm:
Self::SetMethodPolicy(a) => {
let policy = if a.reset {
None
} else {
let role = a
.role
.context("--role is required unless --reset is given")?;
- Some(MethodPolicy { ttl: a.ttl, role })
+ let ttl = a
+ .ttl
+ .context("--ttl is required unless --reset is given")?;
+ Some(MethodPolicy { ttl, role })
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Args, Debug)] | |
| pub struct SetMethodPolicyArgs { | |
| /// Target method name (e.g. `admin_set_proxy`). | |
| #[arg(long, value_name = "NAME")] | |
| method: String, | |
| /// Timelock for this method (ignored with `--reset`). Must be `<=` the target default. | |
| #[arg(long, value_name = "DURATION", value_parser = parse_duration, default_value = "0ns")] | |
| ttl: Nanoseconds, | |
| /// Role required to invoke this method. Required unless `--reset`. | |
| #[arg(long, value_enum, required_unless_present = "reset")] | |
| role: Option<Role>, | |
| /// Remove the override, resetting the method to the target default. | |
| #[arg(long, conflicts_with = "role")] | |
| reset: bool, | |
| } | |
| #[derive(Args, Debug)] | |
| pub struct SetMethodPolicyArgs { | |
| /// Target method name (e.g. `admin_set_proxy`). | |
| #[arg(long, value_name = "NAME")] | |
| method: String, | |
| /// Timelock for this method (ignored with `--reset`). Must be `<=` the target default. | |
| /// Required unless `--reset`. | |
| #[arg(long, value_name = "DURATION", value_parser = parse_duration, required_unless_present = "reset")] | |
| ttl: Option<Nanoseconds>, | |
| /// Role required to invoke this method. Required unless `--reset`. | |
| #[arg(long, value_enum, required_unless_present = "reset")] | |
| role: Option<Role>, | |
| /// Remove the override, resetting the method to the target default. | |
| #[arg(long, conflicts_with_all = ["role", "ttl"])] | |
| reset: bool, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/manager/src/commands/proxy_oracle/governance/create_proposal.rs` around
lines 355 - 369, Update SetMethodPolicyArgs so --ttl is required for non-reset
set-method-policy calls, mirroring the role argument’s required_unless_present =
"reset" gating; remove its zero-duration default so omitted TTL fails
validation, while reset operations may continue omitting it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restructures the proxy-oracle governance contract from a flat 12-variant
Operation(each target op a typed constructor) to a two-class model:Reflexive(ReflexiveOperation)— mutates governance's own state (SetReflexiveTtl,SetTargetDefault,SetMethodPolicy,SetRole,SelfUpgrade).TargetFunctionCall(FunctionCall)— a generic(method_name, args, deposit, gas)call dispatched to the governed proxy oracle.execute_proposalcollapses from a 12-arm match to the reflexive arms plus one dispatched call. Adding a new target op no longer needs a governance-contract upgrade — it's aSetMethodPolicyproposal.Policy model
Per-method TTL and role are table-driven via
GovernancePolicy:self_upgradecan exceedset_role),default_target(long TTL,Admin),method_policieswhitelist of{ ttl, role }overrides (bounded byMAX_METHOD_POLICIES).Resolution: a listed method uses its override; any unlisted method — including one from a future target upgrade — falls back to
default_target, so it can never buy a shorter timelock or lower role than the default. Every override is heldttl <= default_target.ttlon write, authoritative at execute time.Compatibility
LegacyOperationmapping, which also powers legacy-JSON acceptance (#[serde(try_from)]).upgrade_ordering, both orderings).TtlConfig<Kind>trait dropped; governanceevent_version→ 3.0.0 with amethodfield; gatewaygetOperationTtl→getGovernancePolicy,METHODS.mdregenerated.Follow-up
The old create-time payload checks (empty proxy, over-long breaker history) are intentionally not reproduced in the now-opaque generic form — enforcing them at the proxy-oracle contract layer is tracked by ENG-520.
Verification
methods_md_is_up_to_dategreen.upgrade_ordering(2, real pinned v0 blob) and the gateway end-to-end governance test pass.cargo fmt --all --checkandclippy -D warningsclean across all touched crates.🤖 Generated with Claude Code
This change is