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

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

Merged

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Feb 6, 2024

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.

…g' of github.com:pendulum-chain/pendulum into 393-use-fee_per_second-from-asset-metadata-in-xcm-config
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good overall 👍 Few remarks:

  • It might be more interesting not to use the same fee_per_second value for each asset in the test
  • We should also change the xcm config for Foucoco
  • We might want to introduce a default value in case the fee_per_second is not available

runtime/common/src/asset_registry.rs Show resolved Hide resolved
runtime/common/src/lib.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/mock.rs Outdated Show resolved Hide resolved
@gianfra-t
Copy link
Contributor Author

@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
Copy link
Member

ebma commented Feb 20, 2024

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
Copy link
Contributor

bogdanS98 commented Feb 21, 2024

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
Copy link
Contributor Author

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.

node/src/constants/foucoco.rs Outdated Show resolved Hide resolved
runtime/common/src/asset_registry.rs Show resolved Hide resolved
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

I think it's fine now 👍 not sure if other @pendulum-chain/devs want to review too

node/src/chain_spec.rs Outdated Show resolved Hide resolved
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

I just commented some minor things about the comments. I think we can improve it a little but it's not important.

runtime/integration-tests/src/amplitude_tests.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/pendulum_tests.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/amplitude_tests.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/pendulum_tests.rs Outdated Show resolved Hide resolved
@gianfra-t gianfra-t mentioned this pull request Feb 28, 2024
@bogdanS98 bogdanS98 merged commit a402b62 into main Mar 1, 2024
2 checks passed
@bogdanS98 bogdanS98 deleted the 393-use-fee_per_second-from-asset-metadata-in-xcm-config branch March 1, 2024 17:02
ebma added a commit that referenced this pull request Mar 14, 2024
@ebma ebma restored the 393-use-fee_per_second-from-asset-metadata-in-xcm-config branch March 14, 2024 14:19
ebma added a commit that referenced this pull request Mar 14, 2024
Revert "393 use fee per second from asset metadata in xcm config (#410)"

This reverts commit a402b62.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use fee_per_second from asset metadata in XCM config
3 participants