This repository has been archived by the owner on May 21, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend pallet-xcm to pay teleport with non teleported asset #266
Extend pallet-xcm to pay teleport with non teleported asset #266
Changes from 14 commits
85bde1c
35e301e
7f13ca7
e484833
900877f
923b9f7
74db417
08a93cf
38918dd
ac306c0
92e5940
fb6d26b
935f233
f280b8e
575e655
fa7b919
42f9527
3d28e3c
ab8bfbc
41aea01
69fb3fe
55fb0fe
7541ba1
a5c8dd4
baefbba
4477bb8
6aca06b
3aeab64
8d26c7d
d2bac9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this input parameter actually required?
My understanding is that it is always going to be the Relay chain/system parachain native asset hence the user would always set the same input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I see your point, I left it as a parameter as I thought that there might be an use case in which you would like to buy execution with other asset that is handeld but is not the Relay/SP native. For example buying with execution with xUSD on AH. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I agree it makes sense to leave this flexibility for the future. Only thing, it would be good to document this, particularly that the sovereign account in AH will need to have enough balance of the relevant asset for this to work (apart from it having a liquidity pool vs the native asset etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to unbox this earlier. You're gonna use it anyway and reanchoring is more expensive than this conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done but would you care to explain further ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want to perform an expensive operation only to then find out your boxed value had a bad version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and makes sense, feel free to resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering, is it possible to mimic what
InitiateTeleport
does so that we don't leave the native asset trapped in the holding?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we could directly pass
assets
in line 172 instead of the hardcodedforeing_assets
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, on it! Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update:
reanchor
,foreign_assets
is no longer hardcoded.BurnAssets
to mimic this part of theInitiateTeleport
instructioncheck_out
logic for this proxy call to track teleports too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to use
BurnAsssets
instruction. In my opinion this is a solution close to as clean as we can get it. This way there is no trapped assets in the holding whereas the counterpart is expected to be minted in the destination chain. I think the check out logic is then secondary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, shouldn't it be a
InitiateTeleport
instruction instead of theBurnAsset
one ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the need for the
ReceiveTeleportedAsset
instruction .. can you explain ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Sure.
We must take in consideration that:
InitiateTeleport
instruction imprints aReceiveTeleportedAsset
as first instruction of the xcm message that it sends.Barrier
that we are targeting for in Asset Hub isAllowTopLevelPaidExecutionFrom
, this Barrier to pass imposes a certain sequence of Instructions to be received in order as detailed here.do_teleport_assets
fn frompallet-xcm
setsBuyExecution
as the first instruction of the xcm message that is sent as parameter toInitiateTeleport
, therefore the Instruction order to be receive would be:--
Receive
->ClearOrigin
(fromInitiateTeleport
)--
BuyExecution
(from the xcm sent as parameter ofInitiateTeleport
)Leading to the Barrier compliant sequence of
ReceiveTele...
->ClearOrigin
->BuyExecution
However, on Asset Hub we cannot
BuyExecution
with HOP or teleport ROC into it which would trigger theReceiveTeleportedAssets
progression with ROC.There is no Instruction that allows me to prepend a
Withdraw (ROC)
+BuyExecution (ROC)
together with theInitiateTeleport
logic.Therefore I am forced to split the logic of the call in two parts: What should be executed on Trappist and what should be sent to be executed on Asset Hub. I am trying to handcraft the logic as close as possible to the one from
InitiateTeleport
but adding the extra instructions that are needed for the use case. This leads to:execute_xcm
for taking the assets to be teleported on origin viaWithdraw
+Burn
send_xcm
for paying execution with ROC and apply the "teleport" logic on Asset Hub. This is done by theWithdraw
+BuyExecution
+ReceiveTele...
+DepositAsset
that is sent to be executed on Asset Hub.With these Instructions on this combination I try to approach to the
do_teleport_assets
logic but sneaking an execution buy with ROC on AH.I hope this explains both this approach and the rest of the comments, please do let me know if you have any further question or if something doesn't make sense since this is just how I figured out how to work around this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why keeping this ROC around would be a potential exploit. Care to explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet to be explored but initially I was depositing
All
assets on beneficiary, and since at the time ROC is coming from pre funded Trappist's SovereignAccount on Asset Hub, there could be a case in which multiple calls drain the funds from the SovereignAccount into the beneficiary.Limiting the amount of the
proxy_asset
, or demanding at least certain amount of thenative_asset
to be teleported could be a starter to avoid this. Atm, by not depositing all assets and only the foreign/native asset whose equivalent amount has been withdrawn on origin, users couldn't benefit from this exploit. However they could drain the funds by trapping all of them because some people just want to watch the world burn.Changing origin to the from
root
to the sender would totally fix this as the funds of ROC (or any proxy_asset) would have been already owned by the caller.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explained the way things work on my other comment. The funds are always coming from a user, since they pay on the sender chain (via burning). The only potential exploit there is minting more than is burnt. If the mechanism works well, Trappist's SA should never be arbitrarily drained, it will always be the result of derivative tokens being burnt.
Trappist's SA's balance in ROCs = sum(balance in derivative ROC for account in Trappist) <- If this holds then it all works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that, if implemented, would solve this existing vulnerability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The origin should be the account that has the funds it wants to teleport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the one paying for the execution with the proxy asset on destiny chain should be the
SovereignAccount
(SA from now on) of the sender account and not the SA of the chain.As a quick recap, proxy asset indicates which asset should be used to buy execution on destiny, but the funding of the account that is going to pay for that execution with said asset must be done previously to this call.
For simplicity, let's focus on the use case of: origin chain is Trappist, destiny chain is Asset Hub and the proxy asset is ROC.
Funding the SA of Trappist is simple, you can obtain the SA address with this tool and the PARA ID. Then you do a simple
transfer
of ROC on Asset Hub to the address of Trappist's SA. Therefore why I currently setRoot
as origin.However doing so for the SA of the caller origin account from Trappist in Asset Hub, let's call it
Alice
, is not straightforward. Two good solutions would be:TransferAsset
of ROC to{1, X2(Trappist,Alice)}
which is the SA of Trappist's Alice on AH (or anySibling
). However, atm, AH doesn't allow executions.Moving forward, I implemented the
HashedDescription
on Trappist'sLocationToAccountId
and was able to get the address SA of Trappist's Alice, meaning the SA of{1, X2(Trappist,Alice)}
MultiLocation.This allowed me to send ROC on AH to this address and now the call buys execution successfully if we set the origin of the call to Alice instead of Root. However,
{1, X2(Trappist,Alice)}
is not considered aTrusted Teleporter
by AH.Using an alias of Alice's SA to Trappist's SA which is currently a TrustedTeleported would be a increment of privileges on Alice that I do not see fit and also AH doesn't accept any
Aliasers
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the name "proxy asset" is the best to describe what's going on here. The idea of this pallet, and extrinsic, is to teleport one main asset to chain B but withdraw another one on B as well.
It might be called something like
withdraw_and_teleport
.It's just the name "proxy" the one that seems misused, it could be called a "fee asset" and that might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding using
Alice
as the origin of the message, in the end, it's not needed, Alice will already pay up on the sender chain, in this case Trappist, with derivative assets, in this case derivative ROCs.It's perfectly fine to
Withdraw
from Trappist's sovereign account since AH doesn't care about Trappist's internal bookkeeping, it only cares that the whole of Trappist has some amount of ROCs.InitiateReserveWithdraw
handles it in that way.On another note, if you only fund Trappist's sovereign account on AH, you're basically giving up power over those assets and are unable to use them again. You should instead use
DepositReserveAsset
. That will fund the SA but also send a message over to Trappist to mint derivative tokens and send them to Alice over there. That will allow Alice to use this extrinsic to teleport whatever she wants, along with withdrawing the real ROCs for fees.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on the wording, as logic evolved the proxy term that I initially used became outdated, will re work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the
DepositReserveAsset
, will explore the suggestion as it would solve two main points:transfer
which requires them to obtain the address of the SA and adds an extra step of friction.As discussed on our following chat regarding this comment, withdrawn ROC derivatives on Trappist should also be burned and surplus of ROCs on Asset Hub could be refunded to avoid trapping unnecessary users funds.
From usability perspective, users would no longer need to fund SA through a
transfer
and they would need toReserveAssetTransfer
ROCs to their addresses on Trappist to be able to do this teleport.Will implement and update on this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#275 Shows a first approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you use
execute_xcm
instead ofsend_xcm
, to initiate the teleport port operation on the local chain ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed here