Skip to content
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

allow treasury to do reserve asset transfers #1447

Merged
merged 32 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f97b8b4
allow treasury to do reserve asset transfers
samelamin Sep 7, 2023
0cb9bd8
fix for feature-propagation test
samelamin Sep 7, 2023
6950232
fix for fmt-manifest
samelamin Sep 7, 2023
6edfe8c
Update polkadot/runtime/kusama/src/xcm_config.rs
samelamin Sep 7, 2023
1c39018
pr comments
samelamin Sep 7, 2023
cd1a317
remove treasury match from AccountId32Aliases
samelamin Sep 7, 2023
92c19e4
Use TreasuryAccount: Get<AccountId>
samelamin Sep 8, 2023
bc47444
fix for failing builds
samelamin Sep 8, 2023
b0a32df
Update polkadot/runtime/kusama/src/xcm_config.rs
samelamin Sep 12, 2023
fd36d62
PR comments
samelamin Sep 12, 2023
8967478
remove unused variable
samelamin Sep 13, 2023
0900cf6
second round of comments
samelamin Sep 14, 2023
97919f2
Standardize DescribeLocation usage
franciscoaguirre Sep 14, 2023
a20fcf6
build fixes
samelamin Sep 15, 2023
d692513
Merge branch 'master' into allow_treasury_to_send_xcm
samelamin Sep 29, 2023
c43e583
fmt
samelamin Sep 29, 2023
b0bb2b5
Merge branch 'master' into allow_treasury_to_send_xcm
franciscoaguirre Sep 29, 2023
3ed9509
pr comment: rename variable
samelamin Oct 9, 2023
b666a1c
Update polkadot/xcm/xcm-builder/src/location_conversion.rs
samelamin Oct 9, 2023
25da765
Merge branch 'master' into allow_treasury_to_send_xcm
bkchr Oct 9, 2023
6410078
Update polkadot/xcm/xcm-builder/src/location_conversion.rs
samelamin Oct 9, 2023
4ae6087
Update polkadot/xcm/xcm-builder/src/location_conversion.rs
samelamin Oct 9, 2023
920b3ea
Update polkadot/xcm/xcm-builder/src/location_conversion.rs
samelamin Oct 9, 2023
cc1fbc5
pr comments
samelamin Oct 9, 2023
7ce71d1
remove from setup
samelamin Oct 9, 2023
20abdf0
pr comments
samelamin Oct 10, 2023
558e585
add para to para treasury test
samelamin Oct 10, 2023
8d478b9
Merge branch 'master' into allow_treasury_to_send_xcm
samelamin Oct 10, 2023
4de166a
add DescribeTreasuryVoiceTerminal to imports
samelamin Oct 10, 2023
5a42734
revert imports
samelamin Oct 10, 2023
aee64f5
Merge branch 'master' into allow_treasury_to_send_xcm
samelamin Oct 10, 2023
28b4f9e
Merge branch 'master' into allow_treasury_to_send_xcm
samelamin Oct 11, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ polkadot-core-primitives = { path = "../../../../../../polkadot/core-primitives"
polkadot-parachain-primitives = { path = "../../../../../../polkadot/parachain", default-features = false}
polkadot-runtime-parachains = { path = "../../../../../../polkadot/runtime/parachains" }
polkadot-runtime = { path = "../../../../../../polkadot/runtime/polkadot" }
staging-kusama-runtime = { path = "../../../../../../polkadot/runtime/kusama" }
xcm = { package = "staging-xcm", path = "../../../../../../polkadot/xcm", default-features = false}
pallet-xcm = { path = "../../../../../../polkadot/xcm/pallet-xcm", default-features = false}

Expand All @@ -49,4 +50,5 @@ runtime-benchmarks = [
"polkadot-runtime-parachains/runtime-benchmarks",
"polkadot-runtime/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
"staging-kusama-runtime/runtime-benchmarks",
]
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@ pub use frame_support::{
};
pub use integration_tests_common::{
constants::{
asset_hub_kusama::ED as ASSET_HUB_KUSAMA_ED, kusama::ED as KUSAMA_ED, PROOF_SIZE_THRESHOLD,
REF_TIME_THRESHOLD, XCM_V3,
accounts::get_treasury_address, asset_hub_kusama::ED as ASSET_HUB_KUSAMA_ED,
kusama::ED as KUSAMA_ED, PROOF_SIZE_THRESHOLD, REF_TIME_THRESHOLD, XCM_V3,
},
xcm_helpers::{xcm_transact_paid_execution, xcm_transact_unpaid_execution},
AssetHubKusama, AssetHubKusamaPallet, AssetHubKusamaReceiver, AssetHubKusamaSender, Kusama,
KusamaPallet, KusamaReceiver, KusamaSender, PenpalKusamaA, PenpalKusamaAPallet,
PenpalKusamaAReceiver, PenpalKusamaASender, PenpalKusamaB, PenpalKusamaBPallet,
};
pub use parachains_common::{AccountId, Balance};
pub use staging_kusama_runtime::governance::pallet_custom_origins::Origin;
pub use xcm::{
prelude::{AccountId32 as AccountId32Junction, *},
v3::{Error, NetworkId::Kusama as KusamaId},
Expand All @@ -46,6 +47,7 @@ pub const ASSET_MIN_BALANCE: u128 = 1000;
pub const ASSETS_PALLET_ID: u8 = 50;

pub type RelayToSystemParaTest = Test<Kusama, AssetHubKusama>;
pub type RelayToParaTest = Test<Kusama, PenpalKusamaA>;
pub type SystemParaToRelayTest = Test<AssetHubKusama, Kusama>;
pub type SystemParaToParaTest = Test<AssetHubKusama, PenpalKusamaA>;

Expand All @@ -66,6 +68,19 @@ pub fn relay_test_args(amount: Balance) -> TestArgs {
}
}

pub fn relay_to_para_test_args(amount: Balance) -> TestArgs {
TestArgs {
dest: Kusama::child_location_of(PenpalKusamaA::para_id()),
beneficiary: AccountId32Junction { network: None, id: PenpalKusamaAReceiver::get().into() }
.into(),
amount,
assets: (Here, amount).into(),
asset_id: None,
fee_asset_item: 0,
weight_limit: WeightLimit::Unlimited,
}
}

/// Returns a `TestArgs` instance to de used for the System Parachain accross integraton tests
pub fn system_para_test_args(
dest: MultiLocation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
// limitations under the License.

use crate::*;

fn relay_origin_assertions(t: RelayToSystemParaTest) {
type RuntimeEvent = <Kusama as Chain>::RuntimeEvent;

Expand All @@ -35,6 +34,27 @@ fn relay_origin_assertions(t: RelayToSystemParaTest) {
);
}

fn relay_treasury_origin_assertions(t: RelayToParaTest) {
type RuntimeEvent = <Kusama as Chain>::RuntimeEvent;
Kusama::assert_xcm_pallet_attempted_complete(Some(Weight::from_parts(630_092_000, 6_196)));

assert_expected_events!(
Kusama,
vec![
// Amount to reserve transfer is transferred to Parachain's Sovereing account
RuntimeEvent::Balances(
pallet_balances::Event::Transfer { from, to, amount }
) => {
from: *from == t.sender.account_id,
to: *to == Kusama::sovereign_account_id_of(
t.args.dest
),
amount: *amount == t.args.amount,
},
]
);
}

fn system_para_dest_assertions_incomplete(_t: RelayToSystemParaTest) {
AssetHubKusama::assert_dmp_queue_incomplete(
Some(Weight::from_parts(1_000_000_000, 0)),
Expand Down Expand Up @@ -160,6 +180,17 @@ fn system_para_to_para_reserve_transfer_assets(t: SystemParaToParaTest) -> Dispa
)
}

fn treasury_limited_reserve_transfer_assets(t: RelayToParaTest) -> DispatchResult {
<Kusama as KusamaPallet>::XcmPallet::reserve_transfer_assets(
<Kusama as Chain>::RuntimeOrigin::from(Origin::Treasurer),
// t.signed_origin,
bx!(t.args.dest.into()),
bx!(t.args.beneficiary.into()),
bx!(t.args.assets.into()),
t.args.fee_asset_item,
)
}

/// Limited Reserve Transfers of native asset from Relay Chain to the System Parachain shouldn't
/// work
#[test]
Expand Down Expand Up @@ -412,3 +443,30 @@ fn reserve_transfer_asset_from_system_para_to_para() {
.set_dispatchable::<AssetHubKusama>(system_para_to_para_reserve_transfer_assets);
system_para_test.assert();
}

#[test]
fn limited_reserve_transfer_native_asset_from_relay_treasury_to_para() {
let beneficiary_id = PenpalKusamaAReceiver::get();
let amount_to_send: Balance = KUSAMA_ED * 1000;
let test_args = TestContext {
sender: get_treasury_address(),
receiver: beneficiary_id,
args: relay_to_para_test_args(amount_to_send),
};

let mut test = RelayToParaTest::new(test_args);

let sender_balance_before = test.sender.balance;

test.set_assertion::<Kusama>(relay_treasury_origin_assertions);
// TODO: Add assertion for Penpal runtime. Right now message is failing with
// `UntrustedReserveLocation`
test.set_dispatchable::<Kusama>(treasury_limited_reserve_transfer_assets);
test.assert();
//
let sender_balance_after = test.sender.balance;
//
assert_eq!(sender_balance_before - amount_to_send, sender_balance_after);
// TODO: Check receiver balance when Penpal runtime is improved to propery handle reserve
// // transfers
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@

// Substrate
use beefy_primitives::ecdsa_crypto::AuthorityId as BeefyId;
use frame_support::PalletId;
use grandpa::AuthorityId as GrandpaId;
use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
use sp_authority_discovery::AuthorityId as AuthorityDiscoveryId;
use sp_consensus_babe::AuthorityId as BabeId;
use sp_core::{sr25519, storage::Storage, Pair, Public};
use sp_runtime::{
traits::{IdentifyAccount, Verify},
traits::{AccountIdConversion, IdentifyAccount, Verify},
BuildStorage, MultiSignature, Perbill,
};

// Cumulus
use parachains_common::{AccountId, AssetHubPolkadotAuraId, AuraId, Balance, BlockNumber};
use polkadot_parachain_primitives::primitives::{HeadData, ValidationCode};
Expand Down Expand Up @@ -74,6 +74,10 @@ pub mod accounts {
pub const FERDIE_STASH: &str = "Ferdie//stash";
pub const FERDIE_BEEFY: &str = "Ferdie//stash";

pub fn get_treasury_address() -> AccountId {
PalletId(*b"py/trsry").into_account_truncating()
samelamin marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn init_balances() -> Vec<AccountId> {
vec![
get_account_id_from_seed::<sr25519::Public>(ALICE),
Expand All @@ -88,6 +92,7 @@ pub mod accounts {
get_account_id_from_seed::<sr25519::Public>(DAVE_STASH),
get_account_id_from_seed::<sr25519::Public>(EVE_STASH),
get_account_id_from_seed::<sr25519::Public>(FERDIE_STASH),
get_treasury_address(),
]
}
}
Expand Down
1 change: 0 additions & 1 deletion polkadot/runtime/kusama/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,6 @@ parameter_types! {
pub const SpendPeriod: BlockNumber = 6 * DAYS;
pub const Burn: Permill = Permill::from_perthousand(2);
pub const TreasuryPalletId: PalletId = PalletId(*b"py/trsry");

pub const TipCountdown: BlockNumber = 1 * DAYS;
pub const TipFindersFee: Percent = Percent::from_percent(20);
pub const TipReportDepositBase: Balance = 100 * CENTS;
Expand Down
25 changes: 18 additions & 7 deletions polkadot/runtime/kusama/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
//! XCM configurations for the Kusama runtime.

use super::{
parachains_origin, AccountId, AllPalletsWithSystem, Balances, Dmp, Fellows, ParaId, Runtime,
RuntimeCall, RuntimeEvent, RuntimeOrigin, StakingAdmin, TransactionByteFee, WeightToFee,
XcmPallet,
governance::Treasurer, parachains_origin, AccountId, AllPalletsWithSystem, Balances, Dmp,
Fellows, ParaId, Runtime, RuntimeCall, RuntimeEvent, RuntimeOrigin, StakingAdmin,
TransactionByteFee, Treasury, WeightToFee, XcmPallet,
};
use frame_support::{
match_types, parameter_types,
Expand All @@ -39,10 +39,10 @@ use xcm_builder::{
AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, ChildParachainAsNative,
ChildParachainConvertsVia, ChildSystemParachainAsSuperuser,
CurrencyAdapter as XcmCurrencyAdapter, IsChildSystemParachain, IsConcrete, MintLocation,
OriginToPluralityVoice, SignedAccountId32AsNative, SignedToAccountId32,
SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents,
WeightInfoBounds, WithComputedOrigin, WithUniqueTopic,
CurrencyAdapter as XcmCurrencyAdapter, IsChildSystemParachain, IsConcrete,
LocalTreasuryVoiceConvertsVia, MintLocation, OriginToPluralityVoice, SignedAccountId32AsNative,
SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId,
UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic,
};
use xcm_executor::traits::WithOriginFilter;

Expand All @@ -61,6 +61,8 @@ parameter_types! {
pub CheckAccount: AccountId = XcmPallet::check_account();
/// The check account that is allowed to mint assets locally.
pub LocalCheckAccount: (AccountId, MintLocation) = (CheckAccount::get(), MintLocation::Local);
/// The treasury account that is associated with the chain.
pub TreasuryAccount: AccountId = Treasury::account_id();
}

/// The canonical means of converting a `MultiLocation` into an `AccountId`, used when we want to
Expand All @@ -70,6 +72,8 @@ pub type SovereignAccountOf = (
ChildParachainConvertsVia<ParaId, AccountId>,
// We can directly alias an `AccountId32` into a local account.
AccountId32Aliases<ThisNetwork, AccountId>,
// We can directly alias a Treasury voice into a local treasury account.
LocalTreasuryVoiceConvertsVia<TreasuryAccount, AccountId>,
);

/// Our asset transactor. This is what allows us to interest with the runtime facilities from the
Expand Down Expand Up @@ -353,6 +357,8 @@ parameter_types! {
pub const StakingAdminBodyId: BodyId = BodyId::Defense;
// Fellows pluralistic body.
pub const FellowsBodyId: BodyId = BodyId::Technical;
// Treasurer pluralistic body.
pub const TreasurerBodyId: BodyId = BodyId::Treasury;
}

#[cfg(feature = "runtime-benchmarks")]
Expand All @@ -365,6 +371,8 @@ parameter_types! {
pub type LocalOriginToLocation = (
// And a usual Signed origin to be used in XCM as a corresponding AccountId32
SignedToAccountId32<RuntimeOrigin, AccountId, ThisNetwork>,
// Treasury Origin to be used to reserve transfer
TreasurerToPlurality,
);

/// Type to convert the `StakingAdmin` origin to a Plurality `MultiLocation` value.
Expand All @@ -374,6 +382,9 @@ pub type StakingAdminToPlurality =
/// Type to convert the Fellows origin to a Plurality `MultiLocation` value.
pub type FellowsToPlurality = OriginToPluralityVoice<RuntimeOrigin, Fellows, FellowsBodyId>;

/// Type to convert the Treasurer origin to a Plurality `MultiLocation` value.
pub type TreasurerToPlurality = OriginToPluralityVoice<RuntimeOrigin, Treasurer, TreasurerBodyId>;

/// Type to convert a pallet `Origin` type value into a `MultiLocation` value which represents an
/// interior location of this chain for a destination chain.
pub type LocalPalletOriginToLocation = (
Expand Down
4 changes: 2 additions & 2 deletions polkadot/xcm/xcm-builder/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ pub use location_conversion::{
ChildParachainConvertsVia, DescribeAccountId32Terminal, DescribeAccountIdTerminal,
DescribeAccountKey20Terminal, DescribeAllTerminal, DescribeFamily, DescribeLocation,
DescribePalletTerminal, DescribeTerminus, GlobalConsensusConvertsFor,
GlobalConsensusParachainConvertsFor, HashedDescription, ParentIsPreset,
SiblingParachainConvertsVia,
GlobalConsensusParachainConvertsFor, HashedDescription, LocalTreasuryVoiceConvertsVia,
ParentIsPreset, SiblingParachainConvertsVia,
};

mod origin_conversion;
Expand Down
55 changes: 55 additions & 0 deletions polkadot/xcm/xcm-builder/src/location_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,28 @@ impl DescribeLocation for DescribeAccountKey20Terminal {
}
}

/// Create a description of the remote treasury `location` if possible. No two locations should have
/// the same descriptor.
pub struct DescribeTreasuryVoiceTerminal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very cute name.


impl DescribeLocation for DescribeTreasuryVoiceTerminal {
fn describe_location(l: &MultiLocation) -> Option<Vec<u8>> {
match (l.parents, &l.interior) {
(0, X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice })) =>
Some(("Treasury").encode()),
samelamin marked this conversation as resolved.
Show resolved Hide resolved
_ => return None,
samelamin marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

pub type DescribeAccountIdTerminal = (DescribeAccountId32Terminal, DescribeAccountKey20Terminal);

pub type DescribeAllTerminal = (
DescribeTerminus,
DescribePalletTerminal,
DescribeAccountId32Terminal,
DescribeAccountKey20Terminal,
DescribeTreasuryVoiceTerminal,
);

pub struct DescribeFamily<DescribeInterior>(PhantomData<DescribeInterior>);
Expand Down Expand Up @@ -317,6 +332,25 @@ impl<Network: Get<Option<NetworkId>>, AccountId: From<[u8; 32]> + Into<[u8; 32]>
}
}

/// Extracts the `AccountId32` from the passed Treasury plurality if the network matches.
samelamin marked this conversation as resolved.
Show resolved Hide resolved
pub struct LocalTreasuryVoiceConvertsVia<TreasuryAccount, AccountId>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice to add here simple test for this converter

PhantomData<(TreasuryAccount, AccountId)>,
);
impl<TreasuryAccount: Get<AccountId>, AccountId: From<[u8; 32]> + Into<[u8; 32]> + Clone>
ConvertLocation<AccountId> for LocalTreasuryVoiceConvertsVia<TreasuryAccount, AccountId>
{
fn convert_location(location: &MultiLocation) -> Option<AccountId> {
let id: [u8; 32] = match *location {
samelamin marked this conversation as resolved.
Show resolved Hide resolved
MultiLocation {
samelamin marked this conversation as resolved.
Show resolved Hide resolved
parents: 0,
interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }),
} => TreasuryAccount::get().into(),
_ => return None,
};
Some(id.into())
}
}

/// Conversion implementation which converts from a `[u8; 32]`-based `AccountId` into a
/// `MultiLocation` consisting solely of a `AccountId32` junction with a fixed value for its
/// network (provided by `Network`) and the `AccountId`'s `[u8; 32]` datum for the `id`.
Expand Down Expand Up @@ -435,6 +469,9 @@ mod tests {
pub type ForeignChainAliasAccount<AccountId> =
HashedDescription<AccountId, LegacyDescribeForeignChainAccount>;

pub type ForeignChainAliasTreasuryAccount<AccountId> =
HashedDescription<AccountId, DescribeFamily<DescribeTreasuryVoiceTerminal>>;

use frame_support::parameter_types;
use xcm::latest::Junction;

Expand Down Expand Up @@ -925,4 +962,22 @@ mod tests {
};
assert!(ForeignChainAliasAccount::<[u8; 32]>::convert_location(&mul).is_none());
}

#[test]
fn remote_account_convert_on_para_sending_relay_treasury() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably I am missing some context here, I can understand remote_account_convert_on_para - that this converter will be used as a part of LocationToAccountId converter on parachain,
but what does exactly mean sending_relay_treasury?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a badly named test i think, but its meant to be remote treasury on a parachain to another parachain

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, because from the parachain's point of view this:

        MultiLocation {
			parents: 1,
			interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }),
		};

represents Plurality-Treasury-Voice location on relay chain (because parents: 1).

From the parachain's point-of-view a relay chain plurality looks like:

MultiLocation { parents: 1, interior: X1(Plurality { .. }

From the parachain's point-of-view an other parachain plurality looks like:

MultiLocation { parents: 1, interior: X2(Parachain(other_parachain_para_id), Plurality { .. }

So I would suggest to cover at least these two case in this test, to be sure that it works as expected.
Ideally, it should maybe cover more cases:
DescribeTreasuryVoiceTerminal is deployed on relay chain and local treasury location.
DescribeTreasuryVoiceTerminal is deployed on relay chain and parachain treasury location? (not sure about this case)
DescribeTreasuryVoiceTerminal is deployed on parachain and local treasury location.
DescribeTreasuryVoiceTerminal is deployed on parachain and relay chain treasury location.
DescribeTreasuryVoiceTerminal is deployed on parachain and other parachain treasury location.

(but as I said, I dont have full context here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bkontur thanks for this, I've renamed the tests and added another test to cover the para to para treasury conversion

I think the DescribeFamily covers all the other cases but please correct me if I am mistaken.

let mul = MultiLocation {
samelamin marked this conversation as resolved.
Show resolved Hide resolved
parents: 1,
interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }),
};
let actual_description =
ForeignChainAliasTreasuryAccount::<[u8; 32]>::convert_location(&mul).unwrap();

assert_eq!(
[
103, 157, 82, 57, 235, 216, 82, 162, 31, 148, 162, 21, 44, 34, 218, 173, 202, 123,
235, 14, 186, 214, 16, 211, 10, 13, 72, 194, 254, 71, 87, 96
],
actual_description
);
}
}