pendulum-chain / pendulum

GNU General Public License v3.0
43 stars 14 forks source link

Use `fee_per_second` from asset metadata in XCM config #393

Closed ebma closed 3 months ago

ebma commented 6 months ago

With #392 we have a fee_per_second attribute available for each asset. We want to use this value to replace the RelativeValues defined eg here.

An example of an implementation that uses the fee_per_second attribute defined in the asset registry to derive XCM fees can be found here.

annatekl commented 6 months ago

Hey team! Please add your planning poker estimate with Zenhub @b-yap @bogdanS98 @ebma @gianfra-t @TorstenStueber

gianfra-t commented 5 months ago

Hey @pendulum-chain/devs, I have doubts into which route to take to implement this, so far I see two roads:

We could use the fee_per_second value in order to calculate the relative value, but maintain the use of the WeightToFee implementation to get the fee amount in terms of the Native token. This way, we would not really be using the fee_per_second in the sense of it's name, but rather an indicator of the relative value of the token when compared to the native one. Also, we would need to make fee_per_second = 1 for the Native.

The other alternative is to replace completely the use of WeightToFee and implement something like this in our charge_weight_in_fungibles method of ChargeWeightInFungibles, where WEIGHT_PER_SECOND is a constant from Frame.

I don't see any other way to combine these two solutions because the actual baselines to calculate the fees are different. In terms of preference, I prefer the first option since we would still have control of the WeightToFee value, but as negatives we have that it can be confusing the use of the fee_per_second value. WDYT?

Also, both solutions would allow to control the absolute value of the fees for each token, just in different ways.

TorstenStueber commented 5 months ago

@gianfra-t At the end both solutions are equivalent if the parameters are defined appropriately, correct?

Now that we have the fee_per_second parameter in the asset registry, I think it is fair to use it straightforward and use the second solution. I would have taken that as the most obvious way to incorporate the fee_per_second parameter anyway, since weight is just defined as a unit of processing time on a standard machine.

We could change our WeightToFee implementation to also use a fee_per_second parameter we define for the native token. But that would always be one more database lookup for calculating transaction fees, so I think it is okay the way it is. We should just ensure that the fee_per_second for the native asset coincides with the multiplier we use in WeightToFee.

ebma commented 5 months ago

I'd also prefer the second option. I think, using the fee_per_second parameter like this should make it easier to calculate with a script like this compared to treating it like a relative value.

Is there a good reason not to implement it similar to what Centrifuge did, ie. using a struct that implements the orml_traits::FixedConversionRateProvider and then using it as part of a FixedRateAssetRegistryTrader in the Trader of our XCM config? Do we need to keep ChargeWeightInFungibles around or can we just skip it?

By the way, @gianfra-t please don't forget to assign yourself to tickets and move them to 'in development' once you start working on them.

TorstenStueber commented 5 months ago

Looks good to me to use the AssetRegistryTrader trait as the Trader similarly to what Centrifuge does.

gianfra-t commented 5 months ago

Thanks for your inputs! Let's go with the "second option" and adjust fee_per_second accordingly.

As for your question @ebma I think we can also use that other trader, because it behaves in the same way as FirsrAssetTrader , so we will probably end up with a very similar configuration as the link you provided.

ebma commented 3 months ago

Opening this again because we reverted the changes in #431.