docs: proxy oracle#393
Conversation
📝 WalkthroughSummary This PR adds comprehensive Proxy Oracle documentation and runbook, renames CLI signer fields, tweaks CI/docs tooling, and adjusts proxy oracle aggregation logic. What changed
Files of interest
Critical review points (must inspect)
Risk estimate
Recommended reviewers
WalkthroughUpdate docs CI to install mdBook v0.5.2 (remove mdbook-linkcheck and linkcheck config), add Proxy Oracle architecture docs and an operations runbook with navigation and governance/runbook links, rename Changes
Sequence Diagram(s)sequenceDiagram
participant Market as Market
participant Proxy as ProxyOracle (on-chain)
participant Resolver as IdentifierResolver
participant Adapter as Adapter/Fetcher
participant External as ExternalSource (Pyth/RedStone/Transformer)
participant Aggregator as Aggregator
Market->>Proxy: request price(price_id)
Proxy->>Resolver: resolve price_id -> feed definition
Resolver-->>Proxy: feed definition (sources, aggregation, filters)
Proxy->>Adapter: fetch source data for each entry
Adapter->>External: fetch / invoke transformer
External-->>Adapter: source price + confidence / aux data
Adapter-->>Proxy: normalized source responses
Proxy->>Aggregator: apply freshness/filter/min-live-source checks
Aggregator-->>Proxy: aggregate (MedianLow/Priority) -> aggregated price
Proxy-->>Market: return aggregated price
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/contract/proxy-oracle/index.md`:
- Line 72: Change the phrase "on chain" to the hyphenated form "on-chain" in the
sentence about deriving LST price so the compound modifier is consistent; edit
the line containing "the redemption rate is available on chain" to read "the
redemption rate is available on-chain".
In `@tools/manager/src/commands/market/remove.rs`:
- Around line 30-33: The tracing log in the market_remove command uses the old
field name "account_id" causing inconsistent structured logs; update the
tracing::info! call inside market_remove::run to emit signer_id =
%self.signer.signer_id instead of account_id, so it matches the
tracing::instrument fields (and other commands) — locate the tracing::info!
invocation in the run method and change the emitted field name to signer_id.
In `@tools/manager/src/commands/mod.rs`:
- Around line 20-26: The tracing field in the info log is inconsistent: change
the tracing::info! call that currently uses account_id = %signer.signer_id to
use signer_id = %signer.signer_id for consistency with other commands (locate
this near the crate::near::account_exists(&ctx.near, &signer.signer_id).await?
check in tools/manager/src/commands/mod.rs) so the field name matches other
modules like proxy_oracle/remove.rs, redstone_adapter/remove.rs, and
registry/remove.rs.
🪄 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: c8e143a4-a8df-4779-a82f-162d5bfa9a77
📒 Files selected for processing (21)
.github/workflows/documentation.ymldocs/book.tomldocs/src/SUMMARY.mddocs/src/contract/index.mddocs/src/contract/proxy-oracle/index.mddocs/src/contract/proxy-oracle/runbook.mddocs/src/governance.mddocs/src/index.mddocs/src/oracles.mdtools/manager/src/commands/deployment/direct.rstools/manager/src/commands/market/remove.rstools/manager/src/commands/mod.rstools/manager/src/commands/proxy_oracle/remove.rstools/manager/src/commands/recover_nep141.rstools/manager/src/commands/redstone_adapter/remove.rstools/manager/src/commands/registry/deployment/clear.rstools/manager/src/commands/registry/remove.rstools/manager/src/commands/registry/version/add.rstools/manager/src/commands/storage_deposit.rstools/manager/src/util/signer_args.rstools/manager/tests/market.rs
💤 Files with no reviewable changes (1)
- docs/book.toml
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/manager/src/commands/market/remove.rs`:
- Line 33: The tracing::info! call in
tools/manager/src/commands/market/remove.rs currently includes signer_id =
%self.signer.signer_id which is redundant because the tracing span already
records signer_id; update the tracing::info! invocation (the one in the remove
command) to drop the signer_id field and log a simplified message like "Account
does not exist, nothing to do" to match other commands such as delete_account
and RemoveRegistry for consistency.
🪄 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: cbfbb116-b700-4fcd-9894-516533e720b3
📒 Files selected for processing (5)
docs/src/contract/proxy-oracle/index.mddocs/src/contract/proxy-oracle/runbook.mdtools/manager/src/commands/market/remove.rstools/manager/src/commands/mod.rstools/manager/src/commands/registry/remove.rs
…nt-the-high-level-design-as-well-as-the-code
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/contract/proxy-oracle/runbook.md`:
- Around line 295-297: The documentation is ambiguous about aggregation
semantics; update the section describing the aggregation method choices
(references: MedianLow, Priority) to explicitly state what each computes and
when it fails, and add example scenarios: 1) For MedianLow explain it collects
all valid source values, orders them, returns the lower median (or median of
even set behavior), and specify failure conditions (e.g., insufficient valid
sources or outlier filtering rules). 2) For Priority explain it selects the
first valid source based on configured priority/weights (or highest-weighted
available), how weights influence selection and how backups are used when
higher-priority sources are invalid. 3) Add one short concrete example for each
(e.g., combining two live feeds with MedianLow vs preferring a primary feed with
two backups under Priority) so operators can choose appropriately.
- Around line 90-94: Add a security note to the runbook near the "tmplrmgr
proxy-oracle deploy" example mentioning that passing secrets via the
"--secret-key" CLI flag is insecure (visible in shell history and process
listings) and recommending secure alternatives such as using environment
variables, protected key files, or hardware/security modules, and mentioning if
the CLI supports alternative auth methods; reference the example command and
flags ("tmplrmgr proxy-oracle deploy", "--signer-id", "--secret-key") so the
note is placed adjacent to that snippet.
- Around line 155-235: Update the docs to explicitly explain how the
aggregator.method "Priority" uses entries[].weight: state that higher numeric
weight means higher priority, how the aggregator treats equal weights (entries
with the same weight are treated as identical priority and the implementation
falls back to the configured ordering or deterministic tie-breaker—verify and
cite the actual runtime behavior), and give guidance on when to use equal
weights (use when you want no preference between sources) versus differentiated
weights (use e.g., 10/1 or 10/5 to make one source primary and the other a
backup). Also adjust the "Primary Plus Backup Feed" section (and its example) to
either rename it to “Primary with Backup” or change the weights from equal (1,1)
to a clear primary/backup weighting (e.g., primary weight >> backup weight) and
add a short note referencing aggregator.method "Priority" and entries[].weight
so operators know which fields control priority.
🪄 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: 7d6642ef-9806-4a16-90e0-406e87744dcd
📒 Files selected for processing (2)
docs/src/contract/proxy-oracle/runbook.mdtools/manager/src/commands/storage_deposit.rs
royalf00l
left a comment
royalf00l
left a comment
There was a problem hiding this comment.
Review: docs: proxy oracle (PR #393)
Good documentation PR. The architecture doc and operations runbook are well-structured and will be immediately useful for operators and curators. The SignerArgs rename and mdBook upgrade are clean housekeeping changes.
What's Good
- Architecture doc is clear and accurately describes the system. Good use of the rekt.news reference for motivation. The layered freshness controls explanation is helpful.
- Operations runbook is thorough — governance lifecycle, CLI examples, JSON feed definition patterns, verification checklist, and the security note about
--secret-keyleaking via shell history. Priorityaggregation docs with concrete examples (weight 10/5/1 fallback) will save operators from misconfiguration.SignerArgsrename fromaccount_id→signer_idimproves clarity since the field is the signer identity, not always the target account.- mdBook upgrade (v0.4.52 → v0.5.2) with SHA256 hash verification is good security practice.
oracles.mdupdate correctly replaces "There is currently no backup oracle available" with proxy oracle backup support.
Issues
1. Empty section header in runbook (minor)
runbook.md line 134 has a ## Proxy Definition Patterns header with no content — it immediately jumps to ## Aggregation Semantics. Either add content or remove the empty header.
2. Linkcheck removal reduces CI safety
Removing [output.linkcheck] from book.toml and the mdbook-linkcheck CI step means broken links in documentation won't be caught. Was this intentional (e.g., mdbook-linkcheck incompatible with v0.5.2)? If so, consider noting why in the commit message or finding an alternative link checker.
3. Governance section understates initialization risk
The architecture doc says:
"proposed changes are delayed by a configurable TTL"
This is true post-initialization, but the TTL defaults to zero on deployment. The docs should mention that operators must set a non-zero TTL as part of the deployment process. The runbook's "Change Governance TTL" section also doesn't note that the initial TTL is zero and should be set immediately after deployment.
The --execute-immediately flag in the runbook (line 251) is only documented as "only if the created proposal TTL is zero" — it should include a note that this is typically only during initial setup, not normal operations.
4. Risk Controls section could be more specific
The "Remaining risks" in the architecture doc list:
- correlated failures across upstream sources
- bad configuration of feed definitions or weights
- liveness issues if too few sources remain fresh
- operational dependency on the governance process for feed changes
From our prior security analysis, there are more specific risks worth documenting:
- No circuit breaker / inter-source deviation threshold — if one source reports a wildly different price, the aggregation still includes it
- No price bounds / sanity checks — there's no upper/lower limit on reported prices
- Governance TTL can be reduced to zero via
SetActionTtl, permanently removing timelock protection - No emergency bypass — all feed changes require waiting for TTL, even during an active attack
These are known limitations that operators should be aware of. Documenting them sets correct expectations and avoids surprises.
5. min_sources semantic could be clearer
The runbook says min_sources is the "minimum source threshold" but doesn't clarify that it counts sources that pass freshness filtering, not total configured sources. An operator might think "I have 3 sources configured, so min_sources: 2 means I'm safe" — but if 2 sources are stale, only 1 passes the filter and the aggregation returns no price. Worth a one-line clarification.
6. Missing MedianLow confidence-splitting explanation in architecture doc
The architecture doc mentions "Confidence-Aware Aggregation" and says "The proxy aggregates over source confidence bounds, not just point estimates" but doesn't explain how. The runbook does explain it (splits into lower/upper bounds), but the architecture doc should at least summarize: "Each source price is split into [price - confidence, price + confidence] before the median is computed."
This is important because RedStone prices have zero confidence (effectively doubled in the median), which affects the aggregation behavior.
7. Runbook doesn't document Priority + min_sources interaction
With Priority aggregation and min_sources: 2, the proxy requires 2 valid sources but only uses the highest-weight one. This means the backup source must be live even though its value isn't used — min_sources acts as a health check, not a quorum for aggregation. This is a subtle but important behavioral detail for operators.
Code Changes — Clean
The account_id → signer_id rename across signer_args.rs and all consuming files is consistent and complete. The test update in market.rs matches. The SignerArgs::new() constructor usage in clear.rs is a nice cleanup over direct struct construction. No issues found.
Summary
The documentation is solid and fills an important gap. The main suggestions are:
- Fix the empty section header
- Add a deployment note about zero-default TTL
- Document known limitations more specifically (no circuit breaker, no price bounds, TTL can be zeroed)
- Clarify
min_sourcessemantics after filtering - Consider replacing the link checker or documenting why it was removed
Generated by Claude Code
|
@royalf00l linkcheck is not compatible with the latest version of mdbook. I'll create a low-priority task for investigating other broken-link checking solutions. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/contract/proxy-oracle/runbook.md`:
- Around line 146-150: Update the description of the selection semantics for
Priority to clarify it selects among confidence bounds (lower/upper) rather than
raw point values: state that the aggregator considers only surviving sources
(respecting max_age and max_clock_drift), requires min_sources, ranks by weight,
and when a selected source has non-zero confidence it uses the lower bound for
tie-breaking (i.e., Priority chooses between split lower/upper bounds and the
lower bound wins ties), and then mirror this exact clarification in
docs/src/contract/proxy-oracle/index.md so both runbook and index files describe
the same behavior.
- Around line 295-299: The ordered list was broken because the two sub-bullets
starting with "- use `MedianLow`..." and "- use `Priority`..." are not indented
as children of step 4; fix by indenting those two lines (e.g., prefix with four
spaces or two spaces) so they are nested under item 4, and ensure the subsequent
lines remain numbered "5. Create the governance proposal.", "6. Wait for TTL and
execute.", "7. Re-check the live definition." so the main ordered list continues
correctly.
🪄 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: c651d00e-a59c-41f2-9499-c0cfa4681a49
📒 Files selected for processing (3)
common/src/oracle/proxy/aggregator.rsdocs/src/contract/proxy-oracle/index.mddocs/src/contract/proxy-oracle/runbook.md
| - use `MedianLow` to combine all valid live sources into a conservative result | ||
| - use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups | ||
| 5. Create the governance proposal. | ||
| 6. Wait for TTL and execute. | ||
| 7. Re-check the live definition. |
There was a problem hiding this comment.
Fix ordered-list numbering structure to satisfy markdownlint (MD029).
The sub-bullets under step 4 break the ordered list, so lines 297–299 are parsed as a new list and fail lint.
Proposed markdown fix
-4. Choose the aggregation method:
-- use `MedianLow` to combine all valid live sources into a conservative result
-- use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups
-5. Create the governance proposal.
-6. Wait for TTL and execute.
-7. Re-check the live definition.
+4. Choose the aggregation method:
+ - use `MedianLow` to combine all valid live sources into a conservative result
+ - use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups
+5. Create the governance proposal.
+6. Wait for TTL and execute.
+7. Re-check the live definition.📝 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.
| - use `MedianLow` to combine all valid live sources into a conservative result | |
| - use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups | |
| 5. Create the governance proposal. | |
| 6. Wait for TTL and execute. | |
| 7. Re-check the live definition. | |
| 4. Choose the aggregation method: | |
| - use `MedianLow` to combine all valid live sources into a conservative result | |
| - use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups | |
| 5. Create the governance proposal. | |
| 6. Wait for TTL and execute. | |
| 7. Re-check the live definition. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 297-297: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 298-298: Ordered list item prefix
Expected: 2; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 299-299: Ordered list item prefix
Expected: 3; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/contract/proxy-oracle/runbook.md` around lines 295 - 299, The
ordered list was broken because the two sub-bullets starting with "- use
`MedianLow`..." and "- use `Priority`..." are not indented as children of step
4; fix by indenting those two lines (e.g., prefix with four spaces or two
spaces) so they are nested under item 4, and ensure the subsequent lines remain
numbered "5. Create the governance proposal.", "6. Wait for TTL and execute.",
"7. Re-check the live definition." so the main ordered list continues correctly.
carrion256
left a comment
carrion256
left a comment
There was a problem hiding this comment.
We should also disallow ttl to be set back to zero without ratcheting.
|
|
||
| Use this when you want combination across live sources rather than a single preferred source. | ||
|
|
||
| Example: if two equal-weight live sources resolve to `99` and `101`, `MedianLow` returns the lower median of the valid sorted set. With two clean sources, that gives a conservative combined result rather than always preferring one provider. |
There was a problem hiding this comment.
MedianLow is not conservative for all market actions, only collateral valuation. For debt it's the opposite. We should be careful of single biased pricing.
Perhaps when we return prices we should return an additional header containing things like confidence, price scalar, publish time and maybe some source data. This would allow future consumers to be smarter if needed.
There was a problem hiding this comment.
I will update the example to avoid the word "conservative."
It is not possible for the current implementation to return a nonzero confidence interval due to how it normalizes + aggregates prices. However, publish time and the specific price data are returned. Adding a way to retrieve resolution introspection is a feature request.
|
|
||
| - uses only valid sources that survive `max_age` and `max_clock_drift` | ||
| - requires at least `min_sources` valid sources, or it returns no price | ||
| - selects the surviving value with the highest numeric `weight` |
There was a problem hiding this comment.
We are slightly overloading weight here which confused me on our call. For Priority, weight = rank. For aggregation, weight means statistical weight. We should split these into separate names, priority for ordered failover and weight only for statistical aggregation.
| - bad configuration of feed definitions or weights | ||
| - liveness issues if too few sources remain fresh | ||
| - operational dependency on the governance process for feed changes | ||
| - no inter-source deviation check; a bad source can still influence aggregation |
There was a problem hiding this comment.
This is mitigated with windows. We should actually think about per-feed filters, confidence filters, step changes, asset sanity ranges.
There was a problem hiding this comment.
Windows are not implemented yet, so this documentation would be wrong to describe them.
| - proposals can be inspected before execution | ||
| - proposals can be executed or cancelled after review | ||
|
|
||
| The TTL starts at zero on deployment. Operators should set a non-zero TTL during initial setup. |
There was a problem hiding this comment.
We should disallow direct step back to zero. It can only ratchet down. But we should be aware of scope here, we don't want to reinvent governance tightly coupled to the oracle.
There was a problem hiding this comment.
What should the ratchet unit be, and should it be configurable?
Also, this is a feature request, so it should probably be a separate ticket, not implemented on a docs PR.
| - operational dependency on the governance process for feed changes | ||
| - no inter-source deviation check; a bad source can still influence aggregation | ||
| - no absolute price sanity bounds | ||
| - governance can set the TTL back to zero, removing the delay for future proposals |
There was a problem hiding this comment.
This should not happen if we eagerly compute expires_at. I did raise this in the PR.
TTL Reductions themselves should be delayed by the TTL. But this feels like we are rewriting the vault governance.
There was a problem hiding this comment.
Removes the delay for future proposals. The TTL for each proposal is stored in the proposal.
| - no absolute price sanity bounds | ||
| - governance can set the TTL back to zero, removing the delay for future proposals | ||
| - there is no emergency shortcut around the active TTL | ||
|
|
There was a problem hiding this comment.
Old proposals can still lurk around and have their proposals executed at any arbitrary TTL change.
Proposals should be only valid in the world they were submitted in, not brave new worlds.
| ```json | ||
| { | ||
| "aggregator": { | ||
| "method": "MedianLow", |
There was a problem hiding this comment.
Single source should not use medianlow, it should be an explicit passthrough or priority with 1 entry
| ### Confidence-Aware Aggregation | ||
|
|
||
| The proxy aggregates over source confidence bounds, not just point estimates. This is more conservative than a simple passthrough or first-response model. |
There was a problem hiding this comment.
We need more detail here, we should define the aggregated fields. If the market just uses the price scalar that's okay, other consumers need a little bit more.
| @@ -0,0 +1,155 @@ | |||
| # Proxy Oracle Architecture | |||
|
|
|||
| The proxy oracle keeps a stable market-facing oracle interface while letting each feed use one or more underlying sources. | |||
There was a problem hiding this comment.
So after thinking about mutability when @royalf00l mentioned this.
A market that was onboarded expecting a direct spot oracle can silently become a transformed feed, a lower-biased feed, or a backup-selected feed with different behavior. SO what should be mutable?
Source membership/Failover/Freshness = yes
Feed class? I think no. Same for aggregation families, priority/multi-sources
There was a problem hiding this comment.
I agree that we should specifically define:
a) what fields should be mutable and
b) what is the universe of possible new values post mutation (e.g. whitelists)
There was a problem hiding this comment.
Okay, now we're backtracking on what was previously discussed. Originally the feeds were immutable, but now they are mutable only by the owner. If we want a guarantee of immutability, the correct approach given the current implementation is for the owner to abdicate ownership. Because the market configuration is immutable, and we are planning to deploy one price oracle per market, this is a reasonable workflow, I think. If the owner doesn't wish to abdicate governance over two feeds, then they can implement a nonzero TTL.
Regardless, even if we were to implement a feed class restriction, it actually is trivial to circumvent by simply pointing the proxy oracle at another proxy oracle and saying that it's Pyth. I think that it would be a waste of time and resources for us to implement this level of granularity into governance. This partial immutability is easily circumventable by a rogue owner--that's what the TTL is there for. If you don't like a change coming down the pipe, get your funds out now.
If we implement whitelisting, then one of the following must be true:
- Templar maintains a dynamic whitelist onchain. This introduces more points of trust and failure, as well as liability on the part of Templar. Additionally, we would probably have to whitelist entire proxy definitions, or at least full entries, and not just oracle IDs, since, for example, a rogue owner could just proxy asset IDs maliciously, e.g. setting the price ID of USDT to that of BTC, or vice-versa.
- The whitelist is immutably defined by the proxy oracle once upon deployment. This means that if a new oracle arrives, or if an old one breaks/changes address, then the proxy oracle is bricked.
- ...unless the owner is allowed to run upgrade/migration on the contract, but if that is allowed, then we're practically back to the current trust model (no whitelist), because the new code could just ignore the whitelist.
- ...unless the proxy oracle only allows upgrades to code that has been deployed by Templar, but now we inherit all of that liability/trust/failure again, same as case 1.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/src/contract/proxy-oracle/runbook.md (2)
158-166:⚠️ Potential issue | 🟠 MajorSingle-source “passthrough” example should not use
MedianLow.Line 160 says passthrough, but Line 165 uses
MedianLow, which (per Line 137 semantics) resolves via confidence-bound median logic, not strict source passthrough. This can mislead operators configuring single-source feeds.Proposed doc correction
### Single-Source Feed Use for direct passthrough. ```json { "aggregator": { - "method": "MedianLow", + "method": "Priority", "filter": { "max_age": "60000000000", "max_clock_drift": "10000000000", "min_sources": 1 } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/contract/proxy-oracle/runbook.md` around lines 158 - 166, The "Single-Source Feed" example uses the wrong aggregator method: it currently shows "MedianLow" but should demonstrate strict passthrough; update the aggregator configuration in the example to use the "Priority" method instead of "MedianLow" (change the "aggregator"."method" value to "Priority") so the snippet matches the described single-source passthrough semantics.
294-299:⚠️ Potential issue | 🟡 MinorFix broken ordered-list nesting under step 4.
At Line 295 and Line 296, the bullets are not nested under item 4, so Line 297–Line 299 become a new list (MD029).
Proposed markdown fix
4. Choose the aggregation method: -- use `MedianLow` to combine all valid live sources into a conservative result -- use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups + - use `MedianLow` to combine all valid live sources into a conservative result + - use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups 5. Create the governance proposal. 6. Wait for TTL and execute. 7. Re-check the live definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/contract/proxy-oracle/runbook.md` around lines 294 - 299, The ordered-list nesting is broken: the two bullet lines "use `MedianLow` to combine all valid live sources into a conservative result" and "use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups" need to be indented so they are children of the "4. Choose the aggregation method:" list item instead of starting a new top-level list; update the markdown by prefixing those two lines with the appropriate indentation (e.g., four spaces or one tab) so they nest under the "4. Choose the aggregation method:" line and preserve the subsequent numbered steps ("5. Create the governance proposal.", "6. Wait for TTL and execute.", "7. Re-check the live definition.") as continuation of the top-level list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/src/contract/proxy-oracle/runbook.md`:
- Around line 158-166: The "Single-Source Feed" example uses the wrong
aggregator method: it currently shows "MedianLow" but should demonstrate strict
passthrough; update the aggregator configuration in the example to use the
"Priority" method instead of "MedianLow" (change the "aggregator"."method" value
to "Priority") so the snippet matches the described single-source passthrough
semantics.
- Around line 294-299: The ordered-list nesting is broken: the two bullet lines
"use `MedianLow` to combine all valid live sources into a conservative result"
and "use `Priority` to prefer the highest-weight valid source and fall back to
lower-weight backups" need to be indented so they are children of the "4. Choose
the aggregation method:" list item instead of starting a new top-level list;
update the markdown by prefixing those two lines with the appropriate
indentation (e.g., four spaces or one tab) so they nest under the "4. Choose the
aggregation method:" line and preserve the subsequent numbered steps ("5. Create
the governance proposal.", "6. Wait for TTL and execute.", "7. Re-check the live
definition.") as continuation of the top-level list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16cd9b49-c654-47a8-abd8-f71f53fe1678
📒 Files selected for processing (1)
docs/src/contract/proxy-oracle/runbook.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/src/contract/proxy-oracle/runbook.md (1)
294-299:⚠️ Potential issue | 🟡 MinorFix ordered list formatting to resolve markdownlint violation.
The two bullet points under step 4 (lines 295-296) break the ordered list structure because they are not indented as children of step 4. This causes markdownlint MD029 violations on lines 297-299, which are parsed as a new list starting at 5, 6, 7 instead of continuing as 5, 6, 7 from the parent list.
🔧 Proposed fix
4. Choose the aggregation method: -- use `MedianLow` to combine all valid live sources into a conservative result -- use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups + - use `MedianLow` to combine all valid live sources into a conservative result + - use `Priority` to prefer the highest-weight valid source and fall back to lower-weight backups 5. Create the governance proposal. 6. Wait for TTL and execute. 7. Re-check the live definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/contract/proxy-oracle/runbook.md` around lines 294 - 299, Indent the two bullet points that describe aggregation methods (the lines containing `MedianLow` and `Priority`) so they are nested under step 4 of the ordered list; e.g., prefix them with consistent indentation (4 spaces or a tab) so they become children of "4. Choose the aggregation method:" and the subsequent numbered lines remain as steps 5–7 rather than being parsed as a new top-level list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/contract/proxy-oracle/runbook.md`:
- Around line 144-154: Update the "Priority" section to state that selection is
performed over confidence-bound candidates (each source split into lower/upper
bounds) rather than raw point values; explain that the aggregator filters by
max_age, max_clock_drift and min_sources, then chooses the surviving entry with
highest numeric weight, and if that highest-weight source is a single candidate
its lower bound is used as the conservative tie-breaker; ensure this wording
aligns with the confidence-aware aggregation description in index.md and
mentions the relevant symbols: Priority, weight, min_sources, max_age,
max_clock_drift, lower/upper bounds.
---
Duplicate comments:
In `@docs/src/contract/proxy-oracle/runbook.md`:
- Around line 294-299: Indent the two bullet points that describe aggregation
methods (the lines containing `MedianLow` and `Priority`) so they are nested
under step 4 of the ordered list; e.g., prefix them with consistent indentation
(4 spaces or a tab) so they become children of "4. Choose the aggregation
method:" and the subsequent numbered lines remain as steps 5–7 rather than being
parsed as a new top-level list.
🪄 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: 29c7cfd2-4716-4f69-a805-b4ebdeb34d02
📒 Files selected for processing (2)
docs/src/contract/proxy-oracle/index.mddocs/src/contract/proxy-oracle/runbook.md
| ### `Priority` | ||
|
|
||
| - uses only valid sources that survive `max_age` and `max_clock_drift` | ||
| - requires at least `min_sources` valid sources, or it returns no price | ||
| - selects the surviving value with the highest numeric `weight` | ||
| - if a higher-priority source is stale or otherwise filtered out, the next-highest valid source is used | ||
| - if weights are equal, the first surviving entry wins | ||
|
|
||
| Use this when you want a clear primary source with ordered backups. | ||
|
|
||
| Example: if Pyth has weight `10`, RedStone has weight `5`, and a third backup has weight `1`, the proxy uses Pyth when valid, falls back to RedStone when Pyth is invalid, and only uses the third source if both higher-priority sources are unavailable. |
There was a problem hiding this comment.
Clarify that Priority selects from confidence-bound candidates.
Line 148 states "selects the surviving value with the highest numeric weight" but doesn't explain that Priority operates on confidence bounds rather than raw point values. The aggregator splits each source into lower/upper bounds, and for a single highest-weight source, the lower bound is selected as a conservative tie-breaker.
This was flagged in a previous review and the clarification should be consistent with the confidence-aware aggregation description in index.md (lines 58-69).
📝 Suggested clarification
### `Priority`
- uses only valid sources that survive `max_age` and `max_clock_drift`
- requires at least `min_sources` valid sources, or it returns no price
-- selects the surviving value with the highest numeric `weight`
+- selects from each surviving source's confidence-bound candidates (lower and upper bounds)
+- chooses the candidate from the source with the highest numeric `weight`
+- for a single highest-weight source, the lower confidence bound is selected
- if a higher-priority source is stale or otherwise filtered out, the next-highest valid source is used
- if weights are equal, the first surviving entry wins🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/contract/proxy-oracle/runbook.md` around lines 144 - 154, Update the
"Priority" section to state that selection is performed over confidence-bound
candidates (each source split into lower/upper bounds) rather than raw point
values; explain that the aggregator filters by max_age, max_clock_drift and
min_sources, then chooses the surviving entry with highest numeric weight, and
if that highest-weight source is a single candidate its lower bound is used as
the conservative tie-breaker; ensure this wording aligns with the
confidence-aware aggregation description in index.md and mentions the relevant
symbols: Priority, weight, min_sources, max_age, max_clock_drift, lower/upper
bounds.
|
Ok, this PR is on hold until the major refactor to proxy oracles #403 is completed |
3 participants
Removed linkcheck because it is apparently not compatible with this latest version of mdbook.
This change is