Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Multi-Asset Treasury] Exchange Oracle Pallet #202

Closed
tonyalaribe opened this issue Mar 14, 2023 · 10 comments
Closed

[Multi-Asset Treasury] Exchange Oracle Pallet #202

tonyalaribe opened this issue Mar 14, 2023 · 10 comments
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@tonyalaribe
Copy link
Contributor

tonyalaribe commented Mar 14, 2023

Motivation

To support a Multi-Asset treasury where the treasury-pallet needs to spend transactions in assets other than the native asset, there needs to be a way to know how much of the native asset that asset id is worth.

This is useful for book keeping in the treasury, eg to ensure that the total amount of spends irrespective of asset type, is below a particular budget amount which can be configured in a particular asset class (eg the native asset).

Request

Create an Exchange Oracle pallet and a trait that can be used to get the exchange rate for a particular asset pair.
This pallet could rely on configuration or an extrinsic to set the asset pairs.

This is a step towards #98
More info about the larger plan is on this post: #98

@tonyalaribe tonyalaribe changed the title Exchange Oracle Pallet [Multi-Asset Treasury] Exchange Oracle Pallet Mar 14, 2023
@wischli
Copy link

wischli commented Mar 14, 2023

I would like to tackle this!

Create an Exchange Oracle pallet and a trait that can be used to get the exchange rate for a particular asset pair.

It appears to me that the desired trait would basically be BalanceConversion<AssetBalanceOf<T, I>, AssetIdOf<T, I>, BalanceOf<F, T>> reversing the impl for the existing BalanceToAssetBalance struct by swapping the InBalance and OutBalance generics. E.g. adding a new AssetBalanceToBalance struct which converts X units of AssetBalance to Y units of Balance based on the minimum_balance of both assets independent of any oracle conversion rate which is part of the oracle pallet.

However, this trait is inconsistently defined and should be made more generic.

- /// 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>;
}

Then, the Oracle pallet could convert a given amount of AssetBalance to Balance by using assets::types::AssetBalanceToBalance as associated type and a storage map for the conversion in FixedU128:

pub struct Config {
	 /// snip
	 /// From assets::types::AssetBalanceToBalance (see above)
	type AssetBalanceToBalance = BalanceConversion<AssetBalanceOf<T>, AssetIdOf<T>, BalanceOf<T>>,
}
/// Maps an asset to its fixed point representation in the native balance.
pub type ConversionRateToNative =
	StorageMap<_, Blake2_128Concat, T::AssetId, FixedU128, OptionQuery>;


fn asset_balance_to_native(asset: T::AssetId, amount: AssetBalanceOf<T>) -> Option<BalanceOf<T>> {
	ConversionRateToNative::<T>::get(asset)
		.map(|rate| {
			let balance = T::AssetBalanceToBalance::to_balance(amount, asset)?;
			rate.saturating_mul_int(balance)
		})
}

WDYT @tonyalaribe @joepetrowski ?

@burdges
Copy link

burdges commented Mar 14, 2023

A multi-asset treasury is not the original point of #98 really. It just got hijacked.

We'll do a multi-asset treasury eventually I guess, but this probably requires some actual thought and discussion, preferably face-to-face at a parity retreat.

As a rule, oracles are extremely easy targets to attack, so not sure a multi-asset treasury should even touch oracles. If you want to be paid in X then negotiate to be paid in X, like real transactions between companies. If you make the negotiations complex to try to get a few % more, then any humans managing treasury stuff should refuse to deal with you now or in the future.

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/a-better-treasury-system/291/37

@Tomen
Copy link

Tomen commented Mar 15, 2023

Liquidity pools can work as internal oracles. This property can be leveraged to avoid external oracles. Statemine liquidity pools could be used as oracles for price.

The attack vectors that need to be considered are sandwich attacks, as governance action is likely known in advance.

Picking a random block also doesn't guarantee that there will be no attack, as the validator that builds the block still could include malicious transactions

@joepetrowski
Copy link
Contributor

@Tomen and @burdges - I understand your points, but I think you are making some assumptions about how this will be used. @tonyalaribe should write up a more detailed architecture and roadmap of how these issues fit together.

The primary motivation for this pallet derives from the fact that Treasury logic is governance-related, while Treasuries (<-plural!) can hold many assets in many locations. We have different tracks like "{Small, Medium, Big} Spender" that have their boundaries defined in DOT.

The oracle described here only has state (the mapping of conversion ratios) and an API to update the mapping. Of course some governance body or Root itself could update it, but it could also receive periodic XCMs from liquidity pools to update. This part contains no Treasury-related logic; it just allows the Treasury to say its proposals are staying roughly within the track limits.

The actual Treasury execution will be handled by an async PayOverXcm trait, similar to the Salary pallet's. It will assume that the Treasury already has the asset being requested.

If you want to be paid in X then negotiate to be paid in X, like real transactions between companies. If you make the negotiations complex to try to get a few % more, then any humans managing treasury stuff should refuse to deal with you now or in the future.

That's exactly what this does: facilitate negotiating in X on a chain governance) that doesn't really know about X or if the Treasury even has X (because it might be on Assets Hub, e.g.). If the Treasury doesn't have X (or enough X), then payment will fail and you were indeed silly to have done the negotiation.

Liquidity pools can work as internal oracles. This property can be leveraged to avoid external oracles. Statemine liquidity pools could be used as oracles for price.

Yup, indeed. We already considered that this could start with a mapping updated by governance (either some body, like Ecosystem Fellowship, or public referenda), but could also subscribe to periodic updates from Statemint.

@burdges
Copy link

burdges commented Mar 15, 2023

This part contains no Treasury-related logic; it just allows the Treasury to say its proposals are staying roughly within the track limits.

Alright this sounds fine. We'll have really out of date price information though, so not sure what to do.

If the Treasury doesn't have X (or enough X), then payment will fail and you were indeed silly to have done the negotiation.

This does not sound like much of a check against market manipulation though.

We still need actual humans pull the trigger here, right? We already work somewhat that way when the treasury passes funds into multi-sigs who control the actual payout, right? In principle, those people should detect and obstruct market manipulation during payouts, aka liquidity pools being soft targets.

@joepetrowski
Copy link
Contributor

Alright this sounds fine. We'll have really out of date price information though, so not sure what to do.

Well, first I don't think that's so critical. It's there to give people context when they start a referendum. But also not necessarily. A DEX, like @Tomen said, could send an updated table every N blocks (e.g. every 6 hours).

This does not sound like much of a check against market manipulation though.

I don't see what you mean. Sure, if governance itself is manipulating entries, then governance can do other stupid things too.

We still need actual humans pull the trigger here, right?

Pull what trigger?

We already work somewhat that way when the treasury passes funds into multi-sigs who control the actual payout, right?

I think you are thinking specifically of bounties. Treasury proposals just transfer DOT to some AccountId.

@tonyalaribe
Copy link
Contributor Author

@wischli thanks for pointing out the existing BalanceConversion trait. Though it's not clear to me that it covers this usecase. BalanceConversion as it is, seems to focus on converting from a fungibles asset to a given balance. So we have one asset_id and a native balance.
What I imagine is converting from one AssetBalance of a particular asset_id to another AssetBalance of a different asset_id.

For example it could look like:

pub trait BalanceConversion {
	type AssetId: AssetId;
	type Balance: Balance;
	fn quote(asset_id:Self::AssetId, amount: Self::Balance, to_asset_id: Self::AssetId) -> Self::Balance;
}

These ideas are still taking shape and this issue is primarily to track the shaping of these ideas.

@Tomen and @burdges thanks for these points. @joepetrowski has explained quite a bit of the intentions. But like he said, we definitely need a clearer roadmap or architecture of how these issues intersect and shape up a potential multi-treasury. I will share this as soon as I can.

@tonyalaribe
Copy link
Contributor Author

@wischli thinking about this a bit more, I think you're right about simply converting to native balance being sufficient for this usecase, so the existing BalanceConversion trait should works well enough for this multi treasury usecase, especially since we would always only do the conversion to the native asset for rough estimation purposes.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed J0-enhancement labels Aug 25, 2023
@joepetrowski
Copy link
Contributor

Closed by paritytech/substrate#13608

@github-project-automation github-project-automation bot moved this from Backlog to Done in Runtime / FRAME Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Done
Development

No branches or pull requests

8 participants