-
Notifications
You must be signed in to change notification settings - Fork 378
Conversation
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.
Looks good. It's a thumb's up from me once the TODOs in the code are addressed.
Co-authored-by: Squirrel <gilescope@gmail.com>
8f6e013
to
4194b2e
Compare
// Collectives does not recognize a reserve location for any asset. Users must teleport DOT | ||
// where allowed (e.g. with the Relay Chain). | ||
type IsReserve = (); | ||
type IsTeleporter = NativeAsset; // <- should be enough to allow teleportation of DOT |
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 would also allow any other chain to teleport their native token to the collectives parachain. I think to really reduce the liability of the collectives parachain, we really should only be accepting the DOT/KSM token from the relay chain, and nothing else.
parachains/runtimes/collectives/collectives-polkadot/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
…onfig.rs Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
use sp_runtime::AccountId32; | ||
|
||
/// Relay Chain treasury pallet id, used to convert into AccountId | ||
pub const RELAY_TREASURY_PALL_ID: PalletId = PalletId(*b"py/trsry"); |
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.
Someday these should go in a central repository (onchain) of all chain metadatas, so no one needs to hardcode them anymore -- discussed in the parachain summit as well.
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.
At the minimum, can't we move things that are constant like this to polkadot_core_primitives
?
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, @muharem and I discussed several options for this. We agreed that for now hardcode it to avoid waiting for a Polkadot release, but that these can go into crates similar to the Balances (ED, etc.) and be imported.
parachains/runtimes/collectives/collectives-polkadot/src/constants.rs
Outdated
Show resolved
Hide resolved
parachains/runtimes/collectives/collectives-polkadot/src/constants.rs
Outdated
Show resolved
Hide resolved
for ToParentTreasury<TreasuryAcc, TempAcc, T, I> | ||
where | ||
TreasuryAcc: Get<AccountIdOf<T>>, | ||
TempAcc: Get<AccountIdOf<T>>, |
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.
If this is going to be SLASHED_IMBALANCE_ACC_ID
, I don't think it is needed to make it a configurable generic anymore, but as you wish.
/// Relay Chain `TransactionByteFee` / 10 | ||
pub const TransactionByteFee: Balance = 1 * MILLICENTS; |
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 is not really guaranteed, as I recall you have access to relay chain constant, perhaps it can be written 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.
Looks like it's not exported in constants
. Perhaps a good one to add there to just import it.
https://github.com/paritytech/polkadot/blob/release-v0.9.27-1/runtime/polkadot/constants/src/lib.rs
/// Alliance proxy. Allows calls related to the Alliance. | ||
Alliance, | ||
} | ||
impl Default for ProxyType { |
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? shouldn't default be actually the most restrained variant?
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.
Kind of related to your comment on pre-defining common types. The default type is Any
in Polkadot: https://github.com/paritytech/polkadot/blob/release-v0.9.27-1/runtime/polkadot/src/lib.rs#L1134L1138
However, I think it's hard to define "most restrained", because besides Any
and NonTransfer
they tend to be non-overlapping sets of call access.
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 proxies allow addition/modification to less privileged ones, but not more. When creating an anon proxy you would almost always want Any
, otherwise you could never spend from it.
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.
It must be the most permissive value: https://github.com/paritytech/substrate/blob/a1a9b475c354d873f91135ed5fa028ac9ef6a2c4/frame/proxy/src/lib.rs#L127
IMO, we should introduce some custom trait for this, where the function name already expresses on what should be returned.
parachains/runtimes/collectives/collectives-polkadot/src/lib.rs
Outdated
Show resolved
Hide resolved
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 have a bunch of comments that you probably want to address before going forward, but no blockers. LGTM
The CI pipeline was cancelled due to failure one of the required jobs. |
bot merge |
TODOs done, ready for review.
statemint_build_import_queue
to be generic for other paras @gilescope (rename build_import_queue to not be statemint specific #1389)start_statemint_node
@gilescope (rename now shared node creation function #1402)