search

pendulum-chain / pendulum

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

393 use fee per second from asset metadata in xcm config #410

Closed gianfra-t closed 4 months ago

gianfra-t commented 5 months ago

Closes #393.

This PR will replace our previous implementation of the RelativeValue and calculation of the xcm fee by means of WeightToFee.

We will now use the parameter stored in the custom metadata fee_per_second in order to obtain the fee, by replacing altogether the previously implemented trader TakeFirstAssetTrader for AssetRegistryTrader. See the definition for the former and the latter.

Now, the fee is calculated as is defined in FixedRateAssetRegistryTrader here, where: fee = weight/WEIGHT_REF_TIME_PER_SECOND * fee_per_second

Another important difference is that AssetRegistryTrader will attempt to buy from all of the assets exchanged until one is able to do so, while TakeFirstAsset trader will only attempt the first.

Also, if the asset by location is not found in the registry, the trader will error with TooExpensive, this is why in the incorrect incoming asset test we now look for this error.

The FixedConversionRateProvider type that FixedRateAssetRegistryTrader requires will just attempt to get the fee_per_second from the metadata.

Finally, we now require to declare the metadata in our mock chains for integration tests, particularly the fee_per_second value, so that the message is able to execute. For this, we only care that the correct multilocation of the incoming asset exists in the registry, with the corresponding fee_per_second. For simplicity, this value is the same for each asset.

gianfra-t commented 4 months ago

@ebma thanks for pointing out about the xcm config of Foucoco. I didn't consider it. We should keep in mind that any XCM message was allowed before, given the configuration of the barrier AllowUnpaidExecutionFrom<Everything>. Do you think this could break any integration we have right now? For example, with moonbase alpha.

ebma commented 4 months ago

Good question. I doubt that it messes with any of our Foucoco integrations (at the moment just Moonbase Alpha). Should be fine to change it like this. Or what do you think @bogdanS98?

bogdanS98 commented 4 months ago

Good question. I doubt that it messes with any of our Foucoco integrations (at the moment just Moonbase Alpha). Should be fine to change it like this. Or what do you think @bogdanS98?

I think it's fine as long as we have the same XCM config for all runtimes with respect to Barrier, Trader and AssetTransactor. This PR gets all these configurations on the same page and I can't think of any reserved transfer scenario that would be different in Foucoco than in the other runtimes.

gianfra-t commented 4 months ago

That is great to hear @bogdanS98 @ebma. It is nice indeed to have all the same XCM configurations on the chains otherwise testing becomes very difficult. I think I will wait a bit this discussion and then, if you agree with the changes, we can merge.