Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

feat: pallet asset-rate #13608

Merged
merged 21 commits into from
Apr 19, 2023

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Mar 15, 2023

Description

Adds a simple oracle asset-rate pallet which provides means of setting conversion rates for assets to native. These conversion rates are planned to be used to Treasury. Please note that these conversion rates should not be used for the actual payment but just to determine the {Small, Medium, Big} Spender status/track for users which desire to be paid in anything but the native currency. Moreover, it could also be used to softly assert a SpendingPeriods budget has not been exceeded, if that will eventually be necessary for a Multi Asset Treasury.

CUD is enabled via three (potentially) separate origins. The UpdateOrigin could be a DEX which sends an update every N blocks. It was discussed whether {Create, Remove}Origins could be merged. However, I believe we should provide the most flexible granularity and keep all three origins. Consider separate Gov2 track thresholds for adding and removing an asset rate as one argument. In the end, even if CreateOrigin and RemoveOrigin are exactly the same, it's one more line of configuration in the particular runtime.

How is this pallet supposed to be used?

This pallet provides an implementation of ConversionFromAssetBalance which can be plugged into the more generic treasury pallet as an associated type. This enables mapping X amount of balance for an asset to Y amount of native balance. The creation of treasury proposals in Gov2 will always happen on the governance chain (e.g. relay for now, common good in the future). Since such chain does not need to understand multi assets, there needs to be a mechanism to map such balances to the native balance.

TODO

- /// Converts a balance value into an asset balance.
+ /// Provides conversion between two balances.
pub trait BalanceConversion<InBalance, AssetId, OutBalance> {
	type Error;
-	fn to_asset_balance(balance: InBalance, asset_id: AssetId) -> Result<OutBalance, Self::Error>;
+	fn to_balance(balance: InBalance, asset_id: AssetId) -> Result<OutBalance, Self::Error>;
}

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Mar 15, 2023

User @wischli, please sign the CLA here.

@wischli wischli marked this pull request as draft March 15, 2023 14:00
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need create/upgrade/remove? wouldn't an update with Optional param be enough?

frame/treasury-oracle/src/lib.rs Outdated Show resolved Hide resolved
@wischli
Copy link
Contributor Author

wischli commented Mar 16, 2023

Do we really need create/upgrade/remove? wouldn't an update with Optional param be enough?

That could suffice. IMO though, it should be preferred to have better granularity, especially since adding an asset to this pallet's storage can be considered whitelisting them as payout currencies.

Assume the UpdateOrigin will be a DEX in the future, e.g. on Statemin{e,t}. Should this DEX really have the power to add and remove assets which can be requested as payout on a Government chain? IMO it should not, especially when you consider that parachains probably want to have Multi Asset Treasuries as well without necessarily having an independent chain dictate supported spend assets.

Moreover, in the current design the UpdateOrigin does not essantially have to check which assets it actually can update, e.g. the filtering happens in the TreasuryOracle pallet.

@xlc
Copy link
Contributor

xlc commented Mar 16, 2023

especially since adding an asset to this pallet's storage can be considered whitelisting them as payout currencies.

Why the treasury should rely on the oracle pallet to determine an asset is supported? If treasury wants a whitelist, it should implement a whitelist in the treasury. The oracle pallet could be for example used for fee payment, ED amount calculation etc. But if this is the design that this pallet will tightly coupled with treasury, that maybe fine. Is there a design of how all the pallets interact with each other?

Another thing is, any reason why this needs to be treasury-oracle pallet instead of just a generic oracle pallet?

One thing I don't like on how those issues are approached is that I don't see a high level design. I don't see a detailed spec. There is no way for me (and the community) to review them and provide feedbacks. This is Substrate that the pallets are supposed to be generalized and reusable. Currently I need to read the code to guess what you are trying to achieve and as a result I have a lot of questions. Presumably there are a lot of internal discussion and docs but I don't see them so they don't exist for me.

@joepetrowski @bkchr can we improve the process here by requiring someone to write some tech spec of all the new pallets before start coding them?

@joepetrowski
Copy link
Contributor

especially since adding an asset to this pallet's storage can be considered whitelisting them as payout currencies.

Why the treasury should rely on the oracle pallet to determine an asset is supported? If treasury wants a whitelist, it should implement a whitelist in the treasury. The oracle pallet could be for example used for fee payment, ED amount calculation etc. But if this is the design that this pallet will tightly coupled with treasury, that maybe fine. Is there a design of how all the pallets interact with each other?

Another thing is, any reason why this needs to be treasury-oracle pallet instead of just a generic oracle pallet?

@tonyalaribe is writing up a design of how these work together. But the main reason that this is a Treasury-Oracle is that it's really limited to some specific information related to Treasury logic. I would not be opposed to putting this directly in Treasury, in the context that the Treasury logic exists in one place but its assets exist in many. However for a single-asset, single-location Treasury, this would be pointless, so it also makes sense as an extension.

@tonyalaribe
Copy link
Contributor

especially since adding an asset to this pallet's storage can be considered whitelisting them as payout currencies.

Why the treasury should rely on the oracle pallet to determine an asset is supported? If treasury wants a whitelist, it should implement a whitelist in the treasury. The oracle pallet could be for example used for fee payment, ED amount calculation etc. But if this is the design that this pallet will tightly coupled with treasury, that maybe fine. Is there a design of how all the pallets interact with each other?
Another thing is, any reason why this needs to be treasury-oracle pallet instead of just a generic oracle pallet?

@tonyalaribe is writing up a design of how these work together. But the main reason that this is a Treasury-Oracle is that it's really limited to some specific information related to Treasury logic. I would not be opposed to putting this directly in Treasury, in the context that the Treasury logic exists in one place but its assets exist in many. However for a single-asset, single-location Treasury, this would be pointless, so it also makes sense as an extension.

I shared a potential plan here: paritytech/polkadot-sdk#98

frame/treasury-oracle/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury-oracle/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury-oracle/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury-oracle/src/lib.rs Outdated Show resolved Hide resolved
@wischli wischli marked this pull request as ready for review April 3, 2023 12:45
@wischli wischli requested a review from muharem April 3, 2023 12:45
@wischli
Copy link
Contributor Author

wischli commented Apr 3, 2023

@tonyalaribe Please review, thank you!

@wischli wischli changed the title feat: simple treasury oracle feat: pallet asset-rate Apr 3, 2023
@wischli
Copy link
Contributor Author

wischli commented Apr 3, 2023

I renamed the pallet from treasury-oracle to asset-rate as a result of the most recent renaming of pallet-asset-conversion(#12984) as well as @muharem's and @xlc's comments.

An open discussion point is the potential splitting of whitelisting an asset to a separate pallet such that this pallet truly just serves as a convertor without whitelisting implications.

IMO such splitting would be unnecessary as I think it's feasible to assume giving an asset a conversion to native balance relates to whitelisting is as payout currency. In the end, that logic is dependent on the final design of the Multi-Asset Treasury and out of scope of this PR.

Copy link
Contributor

@tonyalaribe tonyalaribe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this to together @wischli. Other than the updates to documentation and how the pallet is portrayed, everything else looks great to me.

@tonyalaribe tonyalaribe added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B0-silent Changes should not be mentioned in any release notes labels Apr 5, 2023
@tonyalaribe
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

frame/asset-rate/Cargo.toml Outdated Show resolved Hide resolved
rust-toolchain.toml Outdated Show resolved Hide resolved
frame/asset-rate/src/lib.rs Outdated Show resolved Hide resolved
frame/asset-rate/src/lib.rs Show resolved Hide resolved
@joepetrowski
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 917f7e3 into paritytech:master Apr 19, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* poc

* fix: remove AssetIdParameter

* tests: add

* docs: add pallet description

* feat: add benches

* refactor: UnknownAssetId

* fix: normalize mock cfg

* fix: benchmarks

* chore: add weights

* refactor: remove storage getter

* chore: apply suggestions from code review

* docs: add native balance to calls

* chore: apply suggestions from code review

* chore: apply ConversionFromAssetBalance

* tests: update balance mock

* chore: apply suggestions from code review

* ci: set publish to false

* docs: fix missing rustdoc

---------

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants