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.
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
Xcm emulator nits #1649
Xcm emulator nits #1649
Changes from 13 commits
3db426d
7aae466
75d966a
d48af15
0df5489
d30a121
20c0e56
7bd3b19
39361b2
c30119c
2453019
f80a3fc
4acfffb
db80302
4478f4b
b51a207
b796743
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 are we assuming that is going to be always
root
? Someone would like to test sending aTransact
with differentorigin
. I think we should either acceptorigin
as an extra argument forsend_transact_to_parachain
or rename method tosend_transact_to_parachain_as_sudo
(better the first option)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.
Well, this is meant for governance-like calls from relay to para with
RuntimeOrigin::root()
andUnpaidExecution
not to repeat the same code everywhere:If we want it make it more general, then yes
origin
and also some "switch" forUnpaidExecution/BuyExecution
wrapper should be added as a parameter. Then we should add it as a generic helper function forChain
trait.I think, for me,
send_transact_to_parachain_as_sudo
means more like there ispallet_sudo
which is not the case.I am inclined to keep it as it is :).
Anyway, anybody else can still create whatever scenario with
[<$chain Pallet>]>::XcmPallet::send
.@NachoPal wdyt?
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.
Didn't realise about the hardcoded
UnpaidExcution
. Are there cases where anUnpaidExecution
is going to work with a different origin thanSuperuser
? If not then I do not see it necessary to have it as function argument.Sorry, I sometimes use
sudo
androot
interchangeably. If it is meant to be for governance-like calls (root
) then we can do:What I am trying to avoid is to impl a method that is only meant to work for
root
,UnpaidExecution
andOrigin::Superuser
with a so generic name -send_transact_to_parachain
- (even if it is properly documented)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.
ok, I renamed to
send_unpaid_transact_to_parachain_as_root
, please, check here: c30119cI also was able to reuse
do_force_create_asset_from_relay_as_root
forforce_create_and_mint_asset
, which looks also much clear now