[Encointer] use XCM V5 to remotely spend funds from encointer treasury accounts on AHK#679
Conversation
This reverts commit 8814143.
ggwpez
left a comment
There was a problem hiding this comment.
Encointer is not a trusted teleporter, or?
it is a trusted teleporter, it's para id is in the sys chains interval, and that's what's used for differentiating trusted teleporters |
|
I think that the comments are addressed. Thanks for pointing this out! |
|
/merge |
|
|
||
| let sender = AccountId::new([1u8; 32]); | ||
| let sender_location_on_target = | ||
| Location::new(1, X2([Parachain(42), AccountId32 { network: None, id: [1; 32] }].into())); |
There was a problem hiding this comment.
@clangenb just an nit, using too low values for parachain could be tricky in some scenarios (bellow 2000 are considered system parachains).
There was a problem hiding this comment.
Good point. I copied this from upstreams Pay unit test. However, the Pay trait assumes free execution on the target parachain, so hence it was a reasonable assumption to put 42 as the parachain. I will make this a higher number when I move these tests upstream. 👍
| pub Timeout: BlockNumber = 5; // 5 blocks | ||
| } | ||
|
|
||
| /// Scenario: |
There was a problem hiding this comment.
@clangenb I didn't walk through these test, but just curios, if we could use emulated tests (EncointerKusama, AssetHubKusama, AssetHubPolkadot, PenpalKusama, PenpalPolkadot) to simulate this? That way we won't need all this mocking stuff.
There was a problem hiding this comment.
| ReportError(QueryResponseInfo { | ||
| destination: destination.clone(), | ||
| query_id, | ||
| max_weight: Weight::zero(), |
There was a problem hiding this comment.
@clangenb is Weight::zero() really working? If I am checking it correctly, it should end up in the: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/lib.rs#L4052-L4064
if weight.any_gt(max_weight) {
let e = Event::NotifyOverweight {
Ok, maybe not that critical if the ReportError does not work, but still :)
There was a problem hiding this comment.
Hmm, interesting. I suspect that the mock that I copied from upstream is lacking some logic here. I will have a closer look here while upstreaming that part. 👍
| // Measured: `733` | ||
| // Estimated: `6196` | ||
| // Minimum execution time: 70_344_000 picoseconds. | ||
| Weight::from_parts(72_445_000, 0) |
There was a problem hiding this comment.
@clangenb Did we run fresh benchmarks for this? I think we should better run maybe as follow up - with mentioned small nits
There was a problem hiding this comment.
Good catch, you are right. This call has not been properly benchmarked! Do I as a mere rank I member have the rights to run the benchmarks?
There was a problem hiding this comment.
... Do I as a mere rank I member have the rights to run the benchmarks?
Yes, you can :)
@bkontur identified minor fixes after #679, which should be fixed. The most important one would be to properly generate the weight of the new dispatchable. Closes #864. <!-- Remember that you can run `/merge` to enable auto-merge in the PR --> <!-- Remember to modify the changelog. If you don't need to modify it, you can check the following box. Instead, if you have already modified it, simply delete the following line. --> - [x] Does not require a CHANGELOG entry
Encointer's treasury beneficiaries would greatly benefit from receiving assets directly on asset hub (especially in the future, as we can assume that CEX exchanges will allow deposits from asset hub). Additionally, we want to give them stability by receiving funds in stable coins (potentially dUSD).
The remote transfer has been inspired by the fellowships approach, however, it is slightly different as encointer has to pay for that execution. Additionally, we need to have a configurable sender account on Asset Hub, as each encointer-community has its own unique treasury account. Similarly to the polkadot-sdk's pay trait, we have introduced the Transfer trait (mostly in order to allow for a configurable sender account on asset hub). Which we intend to upstream after testing and deploying it here.
The idea is that the following happens on asset hub:
Changes in this PR:
SpendAssetandIssueSwapAssetOptionand abstract payment mechanism behind trait encointer/pallets#422 and enable transferring remotely on asset hub from the encointer runtimeTodo: