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

Feature/plmc 315 configure pallet assets to accept usdtusdc from ah #161

Conversation

vstam1
Copy link
Collaborator

@vstam1 vstam1 commented Feb 8, 2024

No description provided.

@vstam1 vstam1 marked this pull request as ready for review February 12, 2024 15:14
Copy link
Contributor

@lrazovic lrazovic left a comment

Choose a reason for hiding this comment

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

Nice job! There are just a few minor details to address. The revamped XCM Config and the Asset Pallet configuration appear to be in good order. I'm currently awaiting @JuaniRios input, particularly on the XCM aspect and the associated tests. Once we have his feedback, we should be all set to proceed with the merge.

integration-tests/src/constants.rs Outdated Show resolved Hide resolved
runtimes/base/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +47 to +48
const DOT_PER_SECOND_EXECUTION: u128 = 0_0_000_001_000; // 0.0000001 DOT per second of execution time
const DOT_PER_MB_PROOF: u128 = 0_0_000_001_000; // 0.0000001 DOT per Megabyte of proof size
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the source/reasoning behind these numbers? (And also the USDT/USDC counterpart)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good question. We definitely have to check these numbers. Question also is if we would like to allow free execution to users transferring assets from AssetHub to Polimec. I used the Barrier configured in the Testnet which allows explicit free execution from the AssetHub location. Which means that messages starting with UnpaidExecution received from AssetHub do not require any weight buying. These numbers would still be used for the transfer back, but for receiving assets, they wouldn't matter.

So question is? How much are we willing to charge for Asset Transfers

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the free execution only for reserve transfers?
We could write a pre-compile macro (i.e proc macro) that does an http request to get the price of PLMC->USD, DOT->USD, assume USDC and USDT == USD, and try to charge the same USD amount we do for PLMC with the other asset.

I don't think it's worth to do that before MVP though.
For now any value within the same order of magnitude as PLMC is good enough, which I believe is what we have atm

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't allow free transfers to anywhere.
The reason we allow unpaid execution from AH is because the AH already charges for execution over there, preventing spam on both our networks.
If we do free execution on our side then we'll be open to spam

integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
integration-tests/src/tests/reserve_backed_transfers.rs Outdated Show resolved Hide resolved
runtimes/base/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +47 to +48
const DOT_PER_SECOND_EXECUTION: u128 = 0_0_000_001_000; // 0.0000001 DOT per second of execution time
const DOT_PER_MB_PROOF: u128 = 0_0_000_001_000; // 0.0000001 DOT per Megabyte of proof size
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the free execution only for reserve transfers?
We could write a pre-compile macro (i.e proc macro) that does an http request to get the price of PLMC->USD, DOT->USD, assume USDC and USDT == USD, and try to charge the same USD amount we do for PLMC with the other asset.

I don't think it's worth to do that before MVP though.
For now any value within the same order of magnitude as PLMC is good enough, which I believe is what we have atm

runtimes/base/src/xcm_config.rs Show resolved Hide resolved
Comment on lines +47 to +48
const DOT_PER_SECOND_EXECUTION: u128 = 0_0_000_001_000; // 0.0000001 DOT per second of execution time
const DOT_PER_MB_PROOF: u128 = 0_0_000_001_000; // 0.0000001 DOT per Megabyte of proof size
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't allow free transfers to anywhere.
The reason we allow unpaid execution from AH is because the AH already charges for execution over there, preventing spam on both our networks.
If we do free execution on our side then we'll be open to spam

Copy link
Contributor

@JuaniRios JuaniRios left a comment

Choose a reason for hiding this comment

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

Since you didn’t provide a description, let me go through what I saw are the changes and you tell me if I missed something important:

  • XCM logic in the tests are unchanged
  • You made the tests logic generic to any asset id on the AH, with a special case in every function for asset 0 which uses the native asset (DOT) instead of pallet-assets.
  • DOT is no longer the only asset for execution on AH, but now is the same asset that you are transferring. This will always work for DOT/USDT/USDC, but if we ever add new assets to test which are not usable for transaction payments then it will fail. Might not be something worth changing.
  • Renaming of references of Statemint to AssetHub
  • Use the base runtime instead of the testnet runtime in the integration tests
  • Add the foreign assets (aka statemintAssets) to base runtime
  • Changed the name of our xcm-executor to polimec-xcm-executor
  • Restricted the xcm_config to only accept DOT,USDC,USDT from AH,
  • Merged the DOT transactor and statemint into one
  • Disallow all pallet-xcm executions, except for transfers of foreign assets back to AH.
  • Give free execution to AH origin
  • Disable teleports

Questions

  • If we teleport PLMC to the AH, what ID will it have on pallet-assets?
    -How does it work to represent another chain’s native assets on AH?

@vstam1
Copy link
Collaborator Author

vstam1 commented Feb 13, 2024

Is the free execution only for reserve transfers?
We could write a pre-compile macro (i.e proc macro) that does an http request to get the price of PLMC->USD, DOT->USD, assume USDC and USDT == USD, and try to charge the same USD amount we do for PLMC with the other asset.

Not sure if this makes sense, as live parachain code will only be up to date to the specific PLMC price while compiling, but price could already be different 5 min later. We could maybe integrate the trader somehow with the oracle. But I think its important to note that this price is only used to calculate fees, which are necessary to prevent spamming of our network. So they are not actually coupled to PLMC, and should probably just be realistic values to prevent spamming (idk 0.1-1 dollar per XCM or something).

@vstam1
Copy link
Collaborator Author

vstam1 commented Feb 13, 2024

  • DOT is no longer the only asset for execution on AH, but now is the same asset that you are transferring. This will always work for DOT/USDT/USDC, but if we ever add new assets to test which are not usable for transaction payments then it will fail. Might not be something worth changing.

Yeah I think its important to note that only assets already on AssetHub are usable as fee payment assets. So if we do not create assets on our side and always first teleport them from AssetHub than we should be able to pay with those assets as well on teleport back.

  • Changed the name of our xcm-executor to polimec-xcm-executor

We already names it polimec-xcm-executor (as far as I know), I only reexported some traits so we do not have to both have xcm-executor and polimec-xcm-executor as dependencies if we want to use the traits.

  • Give free execution to AH origin

This was still something that I took over from test runtime. Should probably discuss if we want this to actually happen.

  • If we teleport PLMC to the AH, what ID will it have on pallet-assets?

Its something I need to investigate. I think it might be placed under the ForeignAssets pallet with its multilocation (so Parachain(our id)) as id. Don't think its relevant before MVP so will investigate later.

-How does it work to represent another chain’s native assets on AH?

Via Its Multilocation as well. So probably by Parachain(x).

@JuaniRios
Copy link
Contributor

Is the free execution only for reserve transfers?
We could write a pre-compile macro (i.e proc macro) that does an http request to get the price of PLMC->USD, DOT->USD, assume USDC and USDT == USD, and try to charge the same USD amount we do for PLMC with the other asset.

Not sure if this makes sense, as live parachain code will only be up to date to the specific PLMC price while compiling, but price could already be different 5 min later. We could maybe integrate the trader somehow with the oracle. But I think its important to note that this price is only used to calculate fees, which are necessary to prevent spamming of our network. So they are not actually coupled to PLMC, and should probably just be realistic values to prevent spamming (idk 0.1-1 dollar per XCM or something).

Yea having a dynamic price would be better, but since its a config type, it can only be changed with a runtime upgrade. (unless we change the logic of the pallet ourselves)

I think there was a pallet coming soon that let you change config types form an extrinsic? @lrazovic

@vstam1
Copy link
Collaborator Author

vstam1 commented Feb 13, 2024

Yea having a dynamic price would be better, but since its a config type, it can only be changed with a runtime upgrade. (unless we change the logic of the pallet ourselves)

Can of course write our own trader configuration (based on one of parity) that actually uses a price_conversion rate trait or something and can call this from the oracle pallet or something. But as I said, I don't think its actually necessary as we are not translating a value to an amount of PLMC. Are directly calculating weight -> assets

@lrazovic
Copy link
Contributor

I think there was a pallet coming soon that let you change config types form an extrinsic? @lrazovic

Yep paritytech/polkadot-sdk#2061

@lrazovic lrazovic self-requested a review February 13, 2024 14:08
@lrazovic lrazovic merged commit 4c26ce0 into main Feb 13, 2024
@JuaniRios JuaniRios deleted the feature/plmc-315-configure-pallet-assets-to-accept-usdtusdc-from-ah branch March 26, 2024 08:19
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.

None yet

3 participants