[Asset Conversion] Incentives Extension

[joepetrowski]

We should have an extension to Asset Conversion pallet to allow rewards for liquidity provision. This could take a number of forms, but the two main options I see are:

1. Make it governance-controlled. As in, governance can allocate some amount of a single asset to a pool and have a reward schedule for it. E.g. the Treasury could allocate some DOT for a DOT<>{stablecoin} pool.
2. Allow anyone to submit any asset for any pool with a schedule. A pool could have multiple reward schedules with it. E.g. anyone can decide that they want to reward liquidity for an X<>Y pool and designate some assets with a schedule for it.

Option (2) is more flexible but can also imagine that it will have a lot of complexity. We may also need a proper implementation of Freeze for assets, which would be useful for other things like asset vesting/voting.

Have discussed with @liamaharon so tagging him.

[xlc]

https://github.com/open-web3-stack/open-runtime-module-library/tree/master/rewards this may or may not be helpful but we are using it for reward handling of the incentives for lp (and other use cases).

[liamaharon]

I've outlined a possible approach below. Let me know how far off this is from what you had in mind @joepetrowski @muharem, and if you have any suggestions :).

Tracking and claiming accumulated rewards

I suggest using the AccumulatedRewardsPerShare approach outlined in this article to track reward allocations to LPs.

It's scalable (constant-time operations) and battle-tested (widely used throughout the Ethereum/EVM ecosystem).

Determining reward tokens per-block

As mentioned by Joe, this would be most easily handled via governance controlling the rewardTokensPerBlock variable, and ensuring the reward 'pot' always has enough tokens available.

Unless there is some pressing reason to allow anyone to donate to the reward pool, then I suggest we start with governance in control and decide whether to extend the implementation to be permissionless in a future iteration.

FRAME implementation

Create a new pallet StakingRewards (name wip, suggestions welcome) to encapsulate this logic. I believe LP tokens are just regular fungible assets(?), so this pallet I think can be general, not only for LP tokens.

StakingRewards allows creating incentive pools for arbitrary fungible tokens, and configuration of the reward rate by governance.

Once a pool is created, LP token holders can then "deposit" their tokens (putting a hold on them) by calling an extrinsic on the pallet and begin accumulating rewards.

If we make this an opt-in process for LP token holders, I don't think the AssetConversion pallet needs any changes.

If we make this an automatic process for LP token holders, we can add optional hooks to the mint/redeem logic in AssetConversion to automatically hold/release tokens. The downside of this approach is that LP tokens would be non-transferable by default.

[joepetrowski]

I haven't looked deeply into AccumulatedRewardsPerShare, but the rest of what you wrote makes sense to me. I like the idea of moving one's LP tokens into a new pallet for reward accumulation.

I'm fine with configuring these rates/pools with governance, but we also will need to make sure the funds come from somewhere. Do you imagine that the pallet pays directly out of the Treasury, or that it gets one-off allocations to distribute?

I believe LP tokens are just regular fungible assets(?), so this pallet I think can be general, not only for LP tokens.

Yes, in the runtime it actually just uses a fresh instance of pallet-assets.

[liamaharon]

Do you imagine that the pallet pays directly out of the Treasury, or that it gets one-off allocations to distribute?

I hadn't considered paying directly out of the Treasury. Are you thinking funds would be pulled directly from the Treasury Pot account? That would be preferable because it wouldn't require the periodic top-ups, we'd just need to be extra vigilant and add guard rails to prevent accidentally draining it if there is a bug 😄.

[joepetrowski]

Yeah exactly, it would be like the opposite of many of the on_unbalanced hooks we have, like what to do with inflation etc. (type X = Treasury). But instead of depositing into the Treasury it would withdraw.

But yes, it is risky. I think these allocations could be large, and something that would only need doing once a year or something. Long term I prefer less manual/governance involvement but it may be better to start with a system that requires explicit top-ups and then transition it to a simpler system that can just pull from the Treasury (within parameters/configuration, obviously).

[muharem]

@liamaharon thank you for sharing the solution. I looked into the article you shared, and it look good to me.

I think we should go for explicit top-ups solution, because otherwise it is risky, and does not look right to let such withdrawals directly from the treasury account. Also when we make it permissioned, to add a staking pool and liquidity for it, we will need this top-up model.

I also would consider to have rewardTokensPerBlock and liquidity pot per pool. This would let us make the pool addition permissioned, and different parties can create and fund different pools. We could even let the addition of a single pool multiple times. Different parties could add the same pool with their own rewards and manage it. If staking results only the freeze on lp token, a user can stake to all available staking pools.

[liamaharon]

Thanks @joepetrowski and @muharem for your feedback, I agree explicit top-ups and segregated pool pots is the most sensible approach here.

I've started scaffolding the

• Documentation
• Config
• Storage
• Calls
• Events
• Logical functions

for a pallet here.

Let me know if you have any feedback. If everything generally looks good I'll get started on the core logic and unit tests.

[muharem]

shared some comments in DM

[liamaharon]

Thanks @muharem! Let me respond here:

Config<Instance>, easier to implement it with first version, less breaks later

To confirm, you mean allow instantiating multiple instances of the pallet? I had designed it so a single instance of the pallet could support all use cases, but happy to allow multiple instances if there is benefit. What do you mean by 'less breaks later'?

I think we are not really benefiting from one registry for lp tokens and for reward tokens, if we separate it would be simpler. Balance, can be the same type for both and different Asset ID types.

Balance is just the accounting unit for the token, (e.g. u64). AssetId is the same for the staking token and reward token. Or do I misunderstand your comment?

PoolId, can be just u32, now sure what we get from making it generic

Yeah I'm happy to do that for simplicity, I made it generic because it is also generic in the asset conversion pallet.

it would be nicer if user can stake(staked_asset_id, amount), to all existing. or may be client app can do it? and may be there are cases when I do not want to stake for some particular pool. Probably implementation will be simpler if we stake by pool_id. Just food for thoughts,

The issue with this is that staking would change from an O(1) operation to O(N). There's probably a way to avoid that, but it would introduce a lot of complexity for a feature we don't care for right now (initially we'll just have a single reward pool per asset).

instead having fixed account id derivation model, you can have config type parameter, and provide a common impl for it. something like in asset-conversion -

polkadot-sdk/substrate/frame/asset-conversion/src/lib.rs

Line 142 in 72f5c46

type PoolLocator: PoolLocator<Self::AccountId, Self::AssetKind, Self::PoolId>;

I'm happy to do this, but curious what the use case is for customising how pool addresses are derived?

to have different admins for different pools, we probably need to store origin within PoolInfo. For the start we probably will allow it only for treasury, but if we do not have it now in PoolInfo, later we will need break api, plus migration. Also this bring one another problem. How pallet origins like Treasurer should be stored in PoolInfo. Usually we just store AccountId. I think we can require any pallet's origin have associated AccountId, and just store that account id. I think it better than storing some complex origin type. So Admin Ensure Origin parameter typr should have AccountId as success result, and for pallet origins we will map them or derive (xcm_config::LocationToAccountId).

Seems like low hanging fruit to remove the need of a complex migration later, SGTM! I did try storing the Origin in the Pool but was having some issues. AccountId sounds better, let me try that.

[muharem]

I will index the the topics in the order they appear.

1. yes, to allow instantiating multiple instances of the pallet. Yes, we can setup if for different purposes with assets registry working with different kind of assets (for example with different pallet assets instances). But I believe that will incapsulate more complexity to registry, more complex AssetId type (or it's matcher for fungibles impl), admin origin. Also perhaps more complex for clients, if one pallet deal with two completely different staking assets (lp tokens form pool and some other derivatives). I mean, less API breaking changes. This minor, but why not?

2. my suggestion:

trait Config {
... 
type Balance: ...
type StakeAssetId: ...
type RewardAssetId: ...
type StakeAsset: fungibles<Balance = Self::Balance, AssetId = Self::StakeAssetId> ...,
type RewardAsset: fungibles<Balance = Self::Balance, AssetId = Self::RewardAssetId> ...,
...
}

For example StakeAssetId can be u32, RewardAssetId can be also (1) u32 or (2) Location. If we have only one registry, AssetId for (1) will be for example AssetKind {Stake(u32), Reward(u32)}, for (2) Location. With separate registries asset id types will be simpler for some cases.

3. In asset conversion it not just increment.

4. I agree, got the same feeling after wrote the first sentence there (: I think clients should manage it.

5. Your account id derivation might be good for our cases (Polkadot/Kusama), but not for others. Someone might have a specific derivation for all system accounts.

[liamaharon]

type StakeAssetId: ...
type RewardAssetId: ...
type StakeAsset: fungibles<Balance = Self::Balance, AssetId = Self::StakeAssetId> ...,
type RewardAsset: fungibles<Balance = Self::Balance, AssetId = Self::RewardAssetId> ...,

This seems unnecessarily complicated to me, I'm probably not familiar with the limitations of the fungible Union logic. Why couldn't there just be a single Asset that's a Union of StakeAsset and RewardAsset?

[muharem]

it can be, but it will result a complex AssetId type. both ways works for me. Let's move further with one registry. If we see it bring more complexity we can decide to separate it to two.

[muharem]

I just realize, that in order to solve this issue, we will need to introduce a pallet extension for pallet-assets to implement fungible::freeze traits

[liamaharon]

Sounds good. I'll start with one, it'll be easiest to understand the trade-offs in practice.

[liamaharon]

I just realize, that in order to solve this issue, we will need to introduce a pallet extension for pallet-assets to implement fungible::freeze traits

Hmm, this would require a seperate issue and PR.

If we really wanted to rush we could still implement this but require users transfer their funds to the sovereign address of the pallet instead of Hold, but not ideal at all. Better if we can get Freezes implemented.

[muharem]

Agree, it's better if we get Freezes implemented