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 23 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.
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 but in this case I didn't initially intend to refund used ROCs to avoid this exploit.
TL:DR since users would be calling this extrinsic from Trappist and the execution in Asset Hub is bought with ROC coming from Trappist's AH
SovereignAccount
, refunding the ROC surplus could be used to drain the SovAcc.A solution that is based on Cisco and Iker's input is presented here but implies users reserve transferring ROC into their Trappist's account.
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.
Yeah I see, but like 275 shows can't you burn the ROC here and then transfer the amount out (partly for fee payment and partly to beneficiary) on AH?
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, so I think that #275 is establishing as the way to go.
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.
Doesn't look trapped?IMO this is hacky. Just debit the user of ROC 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 would need some more explanation here as I do not fully understand where are you suggesting to debit the ROC from. 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.
I mean here on Trappist. You have reserve-backed ROC here, right?
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.
No initially, but yes after #275 , so shall we move forward and merge 275 which solves this ?
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.
Why not just
AllOf
whatever is left?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.
Also, this is depositing the Trappist token, but the comment says (and I believe is correct) that it should deposit ROC.
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 think this is what you want.
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 and initially did it that way but changed to depositing only teleported
foreign_assets
because of this.