-
Notifications
You must be signed in to change notification settings - Fork 54
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
Implement pallet_xcm_core_buyer #452
Conversation
WIP Update polkadot-sdk, include cherry-pick DescribeAllTerminal
Coverage Report@@ Coverage Diff @@
## master tomasz-xcm-coretime +/- ##
=======================================================
- Coverage 65.08% 64.77% -0.31%
+ Files 64 66 +2
+ Lines 9515 9775 +260
=======================================================
+ Hits 6192 6331 +139
+ Misses 3323 3444 +121
|
Overall looks good. I think we are missing typescript tests as well for this |
pallets/xcm-core-buyer/src/lib.rs
Outdated
// They can be easily converted from one another, the difference is that absolute_multilocation | ||
// has an extra "Parachain" junction in the front, using SelfParaId::get() | ||
let interior_multilocation = Self::interior_multilocation(para_id); | ||
let derived_account = Self::absolute_multilocation(interior_multilocation); |
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.
let derived_account = Self::absolute_multilocation(interior_multilocation); | |
let derived_account = Self::relay_relative_multilocation(interior_multilocation); |
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.
We could also rename interior_multilocation
to something like local_relative_multilocation
or local_interior_multilocation
, to emphasize on the fact that the point of view of that multilocation is the local origin.
type GetBlockNumber: Get<u32>; | ||
/// How to convert a `ParaId` into an `AccountId32`. Used to derive the parathread tank | ||
/// account in `interior_multilocation`. | ||
type GetParathreadAccountId: Convert<ParaId, [u8; 32]>; |
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.
Question. Who owns this account? Is it a chain identified by the corresponding para_id
?
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.
yes exactly, it's a chain-derived account based on the paraId.
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.
for now this address is only usable for this purpose. However, in the future we might want the container-chain manager to have some power over this
pub struct BuyCoreCollatorProof<T: Config> { | ||
account: T::AccountId, | ||
// TODO | ||
_signature: (), |
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 signature is required, since we are spending tokens from the parathread tank account 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.
I think we decided not to make this PR bigger with the validation of the signature but @tmpolaczyk correct me if I am wrong. Also in that case let's update the TODO
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.
Yes, the validation logic will be a separate PR so that it can be reviewed 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.
For me the current state of what has to be done in this PR is an approval. Missing pieces that should come as follow-ups are:
- validation logic of the tx
- slot checking (possibly with cooldown) within this pallet
Good job @tmpolaczyk
This pallet allows collators to buy parathread cores on demand.
It uses XCM to send a message to the relay chain, to the extrinsic
OnDemandAssignmentProvider.place_order_allow_death
. The core and the fees are paid using the parathread tank account in the relay chain, which is an account derived through XCM where parathread managers are supposed to send DOT tokens to.It provides 3 extrinsics:
(won't be implemented in this PR) Any collators that are currently assigned to a parathread can call this extrinsic to send an XCM message to the relay chain. We use an
ensure_none
origin to allow collators to call this extrinsic without paying any fees by providing a proof (proof validation is not implemented, so currently all calls will fail). Only 1 XCM message is allowed per tanssi block per parathread.Same as
buy_core
but can only be called by root. Instead of a proof, we simply check that at least one collator is assigned to the para_id. We need this check because it doesn't make sense to buy a core for a parathread that doesn't have any assigned collators, because nobody will produce the block. This is mainly used in tests because the other extrinsic is not implemented yet.Only callable by root, this is used to set a storage item which contains the withdraw amount needed to
BuyExecution
, and theweight_at_most
of theplace_order_allow_death
call. This is a storage item because relay chain weights can change, so we need to be able to adjust them without doing a runtime upgrade.