Feat/restricted transfers closure #1621

mustermeiszer · 2023-11-23T13:40:42Z

Description

This changes closes the restricted transfers features. This will allow accounts to set allowances for their accounts and currencies to where funds are allowed to be sent to (not for CFG though, as we can not restrict pallet-balances).

This is an opt-in restriction meaning

No allowance set → No restrictions
Allowance set → Only allowed receiver can receive funds from that account (on a per currency base)

NOTE:

orml-xtokens has NO storage and pallet-restricted-xtokens mimics the API 1-to-1, hence we can replace the location of orml-xtokens with pallet-restricted-xtokens in the construct_runtime!-macro

Changes and Descriptions

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

NunoAlexandre

The restricted-xtokens pallet looks good 🥳

I left a couple of comments, suggestions or just questions as a first scan but will give it another look either later or tomorrow morning 👍

libs/types/src/locations.rs

pallets/restricted-xtokens/Cargo.toml

pallets/restricted-xtokens/src/mock/mod.rs

NunoAlexandre · 2023-11-23T16:27:19Z

pallets/restricted-xtokens/src/mock/mod.rs

+fn para_a_rreceiver_para_a() -> MultiLocation {
+	MultiLocation::new(
+		0,
+		X1(Junction::AccountId32 {


you don't include the parachain "a" in the multilocation. I guess it doesn't affect functionality but it would be nice to have this in sync with para_a_rreceiver_para_b and generally having it looking like what one would expect it to be (the principle of least surprise)

Why would I do that? The location is generated from the point of view of para_a, right?

NunoAlexandre · 2023-11-23T16:31:21Z

pallets/restricted-xtokens/src/mock/mod.rs

+		let mut a: Vec<u8> = "A".into();
+		a.resize(32, 0);
+		let mut a1: Vec<u8> = "A1".into();
+		a1.resize(32, 0);
+		let mut b: Vec<u8> = "B".into();
+		b.resize(32, 0);
+		let mut b1: Vec<u8> = "B1".into();
+		b1.resize(32, 0);
+		if l == MultiLocation::parent() {
+			return Some(CurrencyId::R);
+		}


I think this is unnecessarily clutter-y. If we would use a GeneralIndex (u128) that we map for each currency, then it's a lot simpler and we could even define that map at the enum definition level (i.e, pub enum CurrencyId { R = 0, A = 1, etc.)

I copy pasta that one from acala. While I agree I think for the given 5 enum variants this logic is fine and I do not expect to change it often in the futre. Wdyt?

pallets/restricted-xtokens/src/tests.rs

pallets/transfer-allowlist/src/lib.rs

pallets/restricted-xtokens/src/lib.rs

pallets/transfer-allowlist/src/lib.rs

pallets/transfer-allowlist/src/mock.rs

lemunozm

Just a couple of comments, but looks good to me! 🚀

pallets/transfer-allowlist/src/lib.rs

pallets/restricted-xtokens/src/lib.rs

wischli

The current extrinsic weights are underestimated. Given that this is a common attack vector, this needs to be fixed.

Apart from that, I also believe that we cannot simply overwrite the OrmlXTokens pallet index with XTokens because we drop the storage.

There a few more nit comments which you can ignore. I have to admit that I only skimmed through the mocks and unit tests.

pallets/restricted-xtokens/Cargo.toml

pallets/restricted-xtokens/src/lib.rs

runtime/altair/src/lib.rs

runtime/centrifuge/src/lib.rs

wischli · 2023-11-24T12:57:04Z

runtime/centrifuge/src/lib.rs

@@ -1994,6 +2015,7 @@ mod __runtime_api_use {

 #[cfg(not(feature = "disable-runtime-api"))]
 use __runtime_api_use::*;
+use cfg_types::locations::Location;


Can you elaborate on this addition?

Clion. will see if can remove

Since I switched to RustRover, I am running into similar issues. Still incredible time saving with the auto imports.

runtime/common/src/lib.rs

runtime/development/src/lib.rs

mustermeiszer · 2023-11-24T13:41:38Z

Apart from that, I also believe that we cannot simply overwrite the OrmlXTokens pallet index with XTokens because we drop the storage.

There is no storage, or am I missing something?

wischli · 2023-11-24T13:53:24Z

Apart from that, I also believe that we cannot simply overwrite the OrmlXTokens pallet index with XTokens because we drop the storage.

There is no storage, or am I missing something?

You are right. Then I suppose we can keep your change. Will close the requests.

pallets/restricted-xtokens/src/tests.rs

runtime/integration-tests/src/generic/cases/liquidity_pools.rs

mustermeiszer

Minus nits from william I think it is good to go. We should address these and then get this in.

Needs: - tests - clean up of warnings

* integration-tests: Add relay chain storage to env, update restricted token tests * restricted-xtokens: Remove xcm-simulator tests * deps: Update restricted-xtokens deps * transfer-allowlist: Use different trait for increasing an account balance in benchmarking setup * integration-tests: Add default constructor for generic envs * integration-test: Add LP eth usdc test via LP pallet * integration-tests: Check for allowance in LP eth usdc test * transfer-allowlist: Use non-default currency ID in benchmarks * clippy: Obey * transfer-allowlist: Drop custom currency ID in mock runtime * transfer-allowlist: Add non-default currency ID for tests

wischli

Thanks for adhering to all my nits and fixing the weight! Looks really good, great work @mustermeiszer and @cdamian!

mustermeiszer · 2023-12-01T11:58:59Z

runtime/altair/src/lib.rs

+			ProxyType::Transfer => {
+				matches!(
+					c,
+					RuntimeCall::XTokens(..) | RuntimeCall::Balances(..) | RuntimeCall::Tokens(..)


@cdamian sit, I forgot something. We should add two more here

RuntimeCall::LiquidityPools(pallet_liquidituy_pool::Call::Transfer{..} | pallet_liquidituy_pool::Call::TransferTrancheTokens{..})

mustermeiszer · 2023-12-01T11:59:55Z

runtime/development/src/lib.rs

+			ProxyType::Transfer => {
+				matches!(
+					c,
+					RuntimeCall::XTokens(..) | RuntimeCall::Balances(..) | RuntimeCall::Tokens(..)


Same as in altair and Centrifuge

mustermeiszer · 2023-12-01T12:01:11Z

/benchmark centrifuge

mustermeiszer · 2023-12-01T12:01:18Z

/benchmark altair

mustermeiszer · 2023-12-01T12:01:27Z

/benchmark development

mustermeiszer requested review from NunoAlexandre, hieronx and wischli as code owners November 23, 2023 13:40

NunoAlexandre reviewed Nov 23, 2023

View reviewed changes

lemunozm reviewed Nov 24, 2023

View reviewed changes

pallets/transfer-allowlist/src/lib.rs Show resolved Hide resolved

pallets/restricted-xtokens/src/lib.rs Outdated Show resolved Hide resolved

wischli requested changes Nov 24, 2023

View reviewed changes

cdamian reviewed Nov 24, 2023

View reviewed changes

pallets/restricted-xtokens/src/tests.rs Outdated Show resolved Hide resolved

cdamian reviewed Nov 27, 2023

View reviewed changes

runtime/integration-tests/src/generic/cases/liquidity_pools.rs Show resolved Hide resolved

cdamian reviewed Nov 27, 2023

View reviewed changes

runtime/integration-tests/src/generic/cases/liquidity_pools.rs Outdated Show resolved Hide resolved

onnovisser mentioned this pull request Nov 27, 2023

Restricted token transfer: add proxy to AO hot wallets centrifuge/apps#1707

Closed

cdamian force-pushed the feat/restricted-transfers-closure branch from b8ca31e to 455cd00 Compare November 30, 2023 15:09

mustermeiszer commented Nov 30, 2023

View reviewed changes

mustermeiszer added 16 commits December 1, 2023 12:34

feat: compiles and is functional.

8eced8e

Needs: - tests - clean up of warnings

chore: clean up tests

a50f867

feat: finish tests

450b916

feat: Adapt allowance to use fungible, restirct native

00b9220

fix: allowlist for dev

4f4fe73

feat: add allowlist to centrifuge

4fc8736

feat: add allowlist Altair

0588195

feat: use fixed versioning for xcm

4547f0b

feat: add to dev

f9a6a00

feat: make dispatch result

018a107

feat: Filter for xcm transfers

462870c

feat: add restricted xtoken to centrifuge

16a95dc

feat: add restricted xtoken altair

a9e321c

feat: add transfer proxy altair

633d3c1

feat: add transfer proxy centrifuge

153fca7

feat: add transfer proxy dev

17e32f8

mustermeiszer and others added 14 commits December 1, 2023 12:34

feat: rename error allowlist

6202dc1

fix: indendation

644875d

fix: doc comments restricted xtokens

498be40

chore: error

b40e606

feat: integration tests wip

800ac01

feat: filter for lp and for tokens

7c4a2ff

feat: use filter for tokens

c8fc791

feat: flter for lp logic and runtime adjustments

ee92176

deps: Update pallet-restricted-xtokens deps

2378b59

deps: Fix integration-tests deps

dc1d46f

deps: Use workspace deps

4642c6a

restricted-xtokens: Update weights

7a3aaf9

restricted-xtokens: Update crate version

ab7528a

cdamian force-pushed the feat/restricted-transfers-closure branch from 51152d7 to ab7528a Compare December 1, 2023 11:35

wischli previously approved these changes Dec 1, 2023

View reviewed changes

mustermeiszer commented Dec 1, 2023

View reviewed changes

cdamian dismissed wischli’s stale review via 5535ee6 December 1, 2023 12:03

runtime: Add LP transfer extrinsics to Transfer proxy type

df59f6a

cdamian force-pushed the feat/restricted-transfers-closure branch from 5535ee6 to df59f6a Compare December 1, 2023 12:29

mustermeiszer merged commit 4c9d5f7 into main Dec 1, 2023
8 checks passed

cdamian deleted the feat/restricted-transfers-closure branch July 30, 2024 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/restricted transfers closure #1621

Feat/restricted transfers closure #1621

mustermeiszer commented Nov 23, 2023 •

edited

Loading

NunoAlexandre left a comment

NunoAlexandre Nov 23, 2023

mustermeiszer Nov 24, 2023

NunoAlexandre Nov 23, 2023

mustermeiszer Nov 24, 2023

lemunozm left a comment

wischli left a comment

wischli Nov 24, 2023

mustermeiszer Nov 24, 2023

wischli Nov 24, 2023

mustermeiszer commented Nov 24, 2023

wischli commented Nov 24, 2023

mustermeiszer left a comment

wischli left a comment

mustermeiszer Dec 1, 2023

mustermeiszer Dec 1, 2023

mustermeiszer commented Dec 1, 2023

mustermeiszer commented Dec 1, 2023

mustermeiszer commented Dec 1, 2023

Feat/restricted transfers closure #1621

Feat/restricted transfers closure #1621

Conversation

mustermeiszer commented Nov 23, 2023 • edited Loading

Description

Changes and Descriptions

Checklist:

NunoAlexandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer commented Nov 24, 2023

wischli commented Nov 24, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer commented Dec 1, 2023

mustermeiszer commented Dec 1, 2023

mustermeiszer commented Dec 1, 2023

mustermeiszer commented Nov 23, 2023 •

edited

Loading