feat(standards): add the price oracle standard (`PriceOracle`, `PriceFeed`, `PriceReaderManager`) by onurinanc · Pull Request #3561 · 0xMiden/protocol

onurinanc · 2026-08-11T11:29:14Z

Closes: #3522

…erface

…n swap

bobbinth · 2026-08-13T20:27:00Z

I would probably simplify this PR: we can start just with the "price oracle" interface. The rest could be added later.

bobbinth

Looks good! Thank you! I left a few small comments inline.

bobbinth · 2026-08-22T01:07:52Z

+# The rate is returned as a `ConversionRate`, rather than a converted amount. 
+# Callers apply it with `fee::convert_amount`, which computes
+# `ceil(amount * num / den)` at 128-bit intermediate precision, so both paths round the same way.


Not for this PR, but we should move convert_amount procedure out of fee module since this procedure is much more general. Don't have a great idea of the right place for it though.

Also (and even less related to this PR), it seems like we have both miden::standards::fee and miden::standards::fees modules. We should probably normalize this somehow.

bobbinth · 2026-08-22T01:14:19Z

Somewhat related to the previous comment: I think ConversionRate struct is much more general than the oracle - and probably more general than just accounts either. So, I think we should move it out - maybe to src/utils?

bobbinth · 2026-08-22T01:16:16Z

+    den: Felt,
+}
+
+impl ConversionRate {


Would it also make sense to add something like convert() procedure which could look like so:

impl ConversionRate { pub fn convert(&self, source: FungibleAsset) -> FungibleAsset { ... } }

And make sure this procedure is semantically equivalent to the fee::convert_amount one?

bobbinth · 2026-08-22T01:17:50Z

+    pub const fn new() -> Self {
+        Self { rate_provider: None }
+    }


nit: is this a useful constructor? I'd imagine we'd always want to construct a price oracle with some rate provider.

PhilippGackstatter

Looks good, I left a few comments.

PhilippGackstatter · 2026-08-22T05:19:24Z

+# WARNING: the body of `get_conversion_rate` MUST NOT change. Consumers reach it over FPI by its
+# MAST root. The body does nothing but read a procedure root from storage and dispatch to it, 
+# so the rate provider can be replaced without changing the root.
+#
+# The wrapper is a stable address, NOT a gate. A dispatch target must itself be an account
+# procedure to be `dyncall` reachable, which also makes it reachable directly over FPI, so a
+# rate provider cannot rely on the wrapper having run first: every guarantee it needs, including
+# its transaction expiration delta, has to be enforced in its own body.


I would replace this with just something like "get_conversion_rate is a stable MAST root that invokes the rate provider dynamically."

Once miden-standards is stable, all procedure roots we have will be stable, so the "MUST NOT change" part is redundant.

The last part isn't necessarily true. The rate provider account procedure could ensure that it is called from the account context (and I think it is best practice to assert this for all dynamically invoked policy procedures that mutate the account - we don't have any so far) so it is not necessarily reachable via FPI.

PhilippGackstatter · 2026-08-22T08:30:54Z

+
+#! Returns the rate converting the source asset into the target asset.
+#!
+#! WARNING: do not edit this procedure since its MAST root is the address consumers resolve against.


nit: related to the above comment, we don't have such comments on other similar procedures so I don't think it's necessary here

PhilippGackstatter · 2026-08-22T08:32:17Z

+#! `den = 0` means the active rate provider cannot price the pair, including when its data is too
+#! stale to rely on. It is returned rather than raised so a caller valuing many assets does not lose
+#! the whole transaction over one. A caller that does not check for it is not left with a wrong
+#! answer either: handing such a rate to `fee::convert_amount` aborts on its zero-denominator
+#! assertion.
+#!
+#! Inputs:  [SOURCE_ASSET_ID, TARGET_ASSET_ID, pad(8)]
+#! Outputs: [num, den, pad(14)]


nit: I would probably make this den = 0 sentinel more explicit by returning a has_conversion_rate boolean flag and saying that num and den are only valid if has_conversion_rate = 1.

PhilippGackstatter · 2026-08-22T08:33:46Z

+#! Inputs:  [SOURCE_ASSET_ID, TARGET_ASSET_ID, pad(8)]
+#! Outputs: [num, den, pad(14)]


Just by reading, it is hard to understand what num and den are and this also applies to the ConversionRate API in general. I would suggest renaming this to something more descriptive, now or in a separate PR.

I'll do this in a follow-up PR while moving the convert_amount to the out of fee module.

PhilippGackstatter · 2026-08-22T08:34:49Z

+#!
+#! The root must belong to a procedure of this account, since `dyncall` can only reach the account's
+#! own procedures.
+#!


I don't remember if we check this in other such procedures, but should we add a check that the account has the procedure?

PhilippGackstatter · 2026-08-22T08:37:31Z

+/// Install it alongside a rate provider on the same account and an
+/// [`Authority`][crate::account::access::Authority], which gates `set_rate_provider`.
+/// `get_conversion_rate` dispatches to whichever provider is registered in
+/// [`PriceOracle::active_rate_provider_slot`], so the pricing can be replaced without changing the
+/// MAST root consumers reach it by.
+///
+/// The wrapper is a stable address, NOT a gate. A dispatch target must itself be an account
+/// procedure to be `dyncall` reachable, which also makes it reachable directly over FPI, so a rate
+/// provider cannot rely on the wrapper having run first: every guarantee it needs, including its
+/// transaction expiration delta, has to be enforced in its own body.


similar comment as before re stability

PhilippGackstatter · 2026-08-22T08:38:10Z

+pub struct PriceOracle {
+    rate_provider: Option<AccountProcedureRoot>,
+}
+


Related to the constructor question from Bobbin, why is this Option and not AccountProcedureRoot?

PhilippGackstatter · 2026-08-22T08:53:07Z

+/// With no rate provider registered the interface aborts rather than dispatching to the empty root,
+/// so an unconfigured oracle cannot be mistaken for one reporting nothing.
+#[tokio::test]
+async fn an_oracle_without_a_rate_provider_aborts() -> anyhow::Result<()> {


nit: I'm not sure we need this test. We don't have it for other such policy managers, afair.

PhilippGackstatter · 2026-08-22T08:53:59Z

+/// The interface's MAST root is the address consumers resolve against, so it must survive changes
+/// to the rate provider and to the rest of the standard.
+///
+/// If this fails, `get_conversion_rate`'s body changed. That is a breaking change for every
+/// consumer that already resolved the old root, not a value to update in passing.
+#[test]
+fn the_interface_root_is_stable() {
+    assert_eq!(
+        PriceOracle::get_conversion_rate_root().mast_root().to_hex(),
+        PINNED_GET_CONVERSION_RATE_ROOT,
+        "the price oracle interface root changed; see the test documentation"
+    );
+}
+
+/// The MAST root of `price_oracle::get_conversion_rate`.
+const PINNED_GET_CONVERSION_RATE_ROOT: &str =
+    "0xed1c07b8a0f3a24addec5ec6651ab0ec5b1e9615d70f00cd1c3f227cbd1a90d9";


related to the other comments, we don't need this, imo

PhilippGackstatter · 2026-08-22T08:54:44Z

+/// The rate provider supplied at genesis is the one the interface dispatches to.
+#[test]
+fn the_component_seeds_the_rate_provider() -> anyhow::Result<()> {
+    let code = rate_providers_code()?;
+    let root = rate_provider_root(&code, "fixed_rate_1500_over_3")?;
+    let oracle = oracle_account(&code, Some(root))?;
+
+    assert_eq!(
+        oracle.storage().get_item(PriceOracle::active_rate_provider_slot())?,
+        *root.mast_root()
+    );
+
+    Ok(())
+}


This is a unit test, so I would move it to standards, or remove it.

mmagician · 2026-08-24T10:10:59Z

+# Exposes the user-facing `get_conversion_rate`, whose MAST root is the address consumers resolve
+# against and must therefore stay stable, plus the authority-gated setter selecting the rate
+# provider it dispatches to.


Suggested change

# Exposes the user-facing `get_conversion_rate`, whose MAST root is the address consumers resolve

# against and must therefore stay stable, plus the authority-gated setter selecting the rate

# provider it dispatches to.

# Exposes the user-facing `get_conversion_rate`, plus the authority-gated setter selecting the rate

# provider it dispatches to.

mmagician · 2026-08-24T10:11:56Z

+#
+# The user-facing price oracle interface: a single procedure returning the rate between two assets.
+#
+# The rate is returned as a `ConversionRate`, rather than a converted amount. 


Suggested change

# The rate is returned as a `ConversionRate`, rather than a converted amount.

# The rate is returned as a `ConversionRate`.

let's only describe current behavior (not absent behavior) in the docs

mmagician · 2026-08-24T10:12:35Z

+# Callers apply it with `fee::convert_amount`, which computes
+# `ceil(amount * num / den)` at 128-bit intermediate precision, so both paths round the same way.


Suggested change

# Callers apply it with `fee::convert_amount`, which computes

# `ceil(amount * num / den)` at 128-bit intermediate precision, so both paths round the same way.

# Callers apply it with `fee::convert_amount`.

I'm not sure these details are relevant for these docs

mmagician · 2026-08-24T10:17:04Z

+pub proc set_rate_provider(rate_provider_proc_root: AccountProcedureRoot)
+    exec.authority::assert_authorized
+    # => [RATE_PROVIDER_PROC_ROOT, pad(12)]
+
+    push.ACTIVE_RATE_PROVIDER_PROC_ROOT_SLOT[0..2] exec.native_account::set_item


I would consider renaming "rate provider" to something else.

When I first read the term "rate provider", I thought that it refers to the entity setting the rates, i.e. the entity feeding the prices to the oracle.

Instead, it refers to the custom/dynamic procedure which performs the price feed read.

Agree, but I'm not sure what should be a better naming. Maybe, ACTIVE_IMPLEMENTATION_PROC_ROOT_SLOT and set_implementation but I'm not sure if the "implementation" here is too generic.

How about ACTIVE_FEED or ACTIVE_PRICE_GETTER?

mmagician

LGTM modulo the rename of rate_provider. I left a suggestion, but maybe others have more ideas?

mmagician · 2026-08-25T05:59:06Z

+pub proc set_rate_provider(rate_provider_proc_root: AccountProcedureRoot)
+    exec.authority::assert_authorized
+    # => [RATE_PROVIDER_PROC_ROOT, pad(12)]
+
+    push.ACTIVE_RATE_PROVIDER_PROC_ROOT_SLOT[0..2] exec.native_account::set_item


How about ACTIVE_FEED or ACTIVE_PRICE_GETTER?

onurinanc · 2026-08-26T14:42:14Z

Related to this comment: #3561 (comment) about renaming rate_provider:

ACTIVE_RATE_PROVIDER -> ACTIVE_PRICE_GETTER seems good to me.

Do you have any other suggestions before making the changes? @bobbinth @PhilippGackstatter

Fumuran

Great work, thank you!

onurinanc added 4 commits August 10, 2026 08:43

add price oracle

d16e451

feat(standards): rework the price oracle around a conversion rate int…

7ec917f

…erface

test(standards): prove the oracle interface survives an implementatio…

9a76ebe

…n swap

merge next

5d44691

onurinanc added 7 commits August 19, 2026 16:07

Merge remote-tracking branch 'origin/next' into feat-price-oracle

cb951de

refactor(standards): simplify the PR with just price oracle interface

cf25770

refactor(standards): drop the timestamp from the oracle interface

79cee8a

rename the implementation slot

3e1f291

fix comments

3bc71bf

fix comments

98a6140

merge

1c7bef8

onurinanc marked this pull request as ready for review August 21, 2026 06:05

onurinanc requested review from PhilippGackstatter and bobbinth August 21, 2026 06:12

bobbinth reviewed Aug 22, 2026

View reviewed changes

PhilippGackstatter reviewed Aug 22, 2026

View reviewed changes

Merge remote-tracking branch 'origin' into feat-price-oracle

7f47d08

mmagician reviewed Aug 24, 2026

View reviewed changes

apply suggestions

715ce24

mmagician approved these changes Aug 25, 2026

View reviewed changes

mmagician requested a review from Fumuran August 27, 2026 12:37

Fumuran approved these changes Aug 28, 2026

View reviewed changes

		#! Inputs: [SOURCE_ASSET_ID, TARGET_ASSET_ID, pad(8)]
		#! Outputs: [num, den, pad(14)]

	# The rate is returned as a `ConversionRate`, rather than a converted amount.
	# The rate is returned as a `ConversionRate`.

		# Callers apply it with `fee::convert_amount`, which computes
		# `ceil(amount * num / den)` at 128-bit intermediate precision, so both paths round the same way.

Uh oh!

Conversation

onurinanc commented Aug 11, 2026

Uh oh!

bobbinth commented Aug 13, 2026

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmagician left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onurinanc commented Aug 26, 2026

Uh oh!

Fumuran left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants