Register origin #174

yrong · 2024-09-03T16:22:27Z

No description provided.

alistair-singh · 2024-09-03T23:43:52Z

Are we trying to avoid an SDK release? or a runtime release? or be fully decoupled from both?

yrong · 2024-09-04T00:00:27Z

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/bridge_to_ethereum_config.rs

@@ -55,6 +56,7 @@ pub type SnowbridgeExporter = EthereumBlobExporter<
 // Ethereum Bridge
 parameter_types! {
 	pub storage EthereumGatewayAddress: H160 = H160(hex_literal::hex!("EDa338E4dC46038493b885327842fD3E301CaB39"));
+	pub storage EnableRegisterToken: bool = false;


Set this flag to be true through System::set_storage when code has been audited.

yrong · 2024-09-04T00:11:08Z

Are we trying to avoid an SDK release? or a runtime release? or be fully decoupled from both?

Yeah, try to be fully decoupled from both. Mentioned in #174 (comment)

acatangiu · 2024-09-04T07:10:47Z

I'm not convinced the added complexity is worth it. Changing storage still requires root/whitelisted referenda.

It's also frowned upon in OpenGov any time we're "arbitrarily" changing storage using Root. You could create an ensure_root call to enable/disable this which would "look" better in OpenGov (not messing with storage directly).

But I would still not do it, more code to maintain for little to no added benefit. Still need root referenda. At least with runtime upgrade we could batch more things in same upgrade referenda.

yrong · 2024-09-04T08:44:59Z

I'm not convinced the added complexity is worth it. Changing storage still requires root/whitelisted referenda.

I would assume it's much easier to do a set_storage than upgrading pallet logic. If that's not the case, then yes the added complexity is not worthwhile.

yrong · 2024-09-04T08:53:00Z

But I would still not do it, more code to maintain for little to no added benefit. Still need root referenda. At least with runtime upgrade we could batch more things in same upgrade referenda.

That's a good point. @acatangiu Does that mean you're fine with the current impl the sudo call and we make a runtime upgrade to make it permissionless after audited?

acatangiu · 2024-09-04T09:46:23Z

But I would still not do it, more code to maintain for little to no added benefit. Still need root referenda. At least with runtime upgrade we could batch more things in same upgrade referenda.

That's a good point. @acatangiu Does that mean you're fine with the current impl the sudo call and we make a runtime upgrade to make it permissionless after audited?

yes, that will be good

acatangiu · 2024-09-05T13:43:18Z

bridges/snowbridge/pallets/system/src/lib.rs

@@ -134,6 +135,28 @@ where
 	No,
 }

+pub struct EnsureOriginWithControlFlag<T, F, A>(core::marker::PhantomData<(T, F, A)>);


not sure we need this somewhat complicated struct..

Instead of switching the boolean generic param on this struct, we can start with simple EnsureRoot struct, then after audit switch (in runtime config) to simple EnsureSigned struct.

acatangiu · 2024-09-05T13:46:28Z

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/bridge_to_ethereum_config.rs

@@ -188,6 +190,8 @@ impl snowbridge_pallet_system::Config for Runtime {
 	type Helper = ();
 	type DefaultPricingParameters = Parameters;
 	type InboundDeliveryCost = EthereumInboundQueue;
+	type RegisterTokenOrigin =
+		EnsureOriginWithControlFlag<Runtime, EnableRegisterToken, TreasuryAccount>;


does TreasuryAccount even have any funds on BridgeHub?

better to just make the tx free for Root rather than try to pay from Treasury account on BH

acatangiu · 2024-09-05T13:51:23Z

bridges/snowbridge/pallets/system/src/lib.rs

@@ -175,6 +198,8 @@ pub mod pallet {

 		#[cfg(feature = "runtime-benchmarks")]
 		type Helper: BenchmarkHelper<Self::RuntimeOrigin>;
+
+		type RegisterTokenOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = AccountIdOf<Self>>;


maybe

Suggested change

type RegisterTokenOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = AccountIdOf<Self>>;

type RegisterTokenOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Self::RuntimeOrigin>;

then you could:

let pays_fee = match origin { RawOrigin::Root => PaysFee::<T>::No, RawOrigin::Signed(who) => PaysFee::<T>::Yes(who),

and avoid having to use special account for Root case

vgeddes · 2024-09-06T12:29:00Z

bridges/snowbridge/pallets/system/src/lib.rs

@@ -617,12 +642,12 @@ pub mod pallet {
 			location: Box<VersionedLocation>,
 			metadata: AssetMetadata,
 		) -> DispatchResult {
-			ensure_root(origin)?;
+			let who = T::RegisterTokenOrigin::ensure_origin(origin)?;


I've realized that if we make token registration permissionless, the origin should be an XCM location (i.e Transact.OriginKind = Xcm), and we should verify that the origin location is an immediate parent of the token location.

Otherwise any random user can register a token with incorrect or misleading metadata.

For example if the token location is /Para(2048)/PalletInstance(7)/GeneralIndex(1), then the only origin location allowed to register it is /Para(2048)/PalletInstance(7).

How would you register a token like KILT?

By sending an XCM directly from KILT parachain? That would require they setup an HRMP channel with bridge hub just for registration and they have an extrinsic/governance to send_xcm directly from the KILT origin.

Impersonating KILT using governance from the relaychain.

I think we should just stick to root for now as all options require governance and we can change the registration model later.

Yeah good point - another idea I had was to move the registration extrinsic to AH which of course can read the asset metadata directly from storage.

but for now, yes, we should stick with root

This reverts commit c699503.

vgeddes

Needs substantial changes due to the vulnerability in #174 (comment)

acatangiu · 2024-09-06T15:29:18Z

Needs substantial changes due to the vulnerability in #174 (comment)

You can make it very simple for now, just have simple EnsureRoot struct that allows only root and we use that. We can defer finding the right permissionless filter for later. The important part now is to have configurable filter type in the pallet that we can change later without having to change the pallet.

yrong requested a review from vgeddes September 3, 2024 16:22

yrong commented Sep 4, 2024

View reviewed changes

yrong requested a review from alistair-singh September 4, 2024 00:11

yrong mentioned this pull request Sep 4, 2024

Transfer Polkadot-native assets to Ethereum paritytech/polkadot-sdk#5546

Merged

acatangiu reviewed Sep 5, 2024

View reviewed changes

vgeddes reviewed Sep 6, 2024

View reviewed changes

yrong added 4 commits September 6, 2024 14:42

Revert "Revert "EnsureRootOrSigned""

2207d4a

This reverts commit c699503.

EnsureOriginWithControlFlag

75a19c3

Add test

14deae8

Fix test

dfa5f83

alistair-singh force-pushed the polkadot-native-assets-custom-origin branch from 3f0c982 to dfa5f83 Compare September 6, 2024 12:44

Use EnsureRoot directly

8337f9b

yrong changed the title ~~Custom origin with control flag~~ Register origin Sep 6, 2024

Cleanup

1314a18

vgeddes requested changes Sep 6, 2024

View reviewed changes

acatangiu approved these changes Sep 6, 2024

View reviewed changes

alistair-singh approved these changes Sep 6, 2024

View reviewed changes

claravanstaden approved these changes Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register origin #174

Register origin #174

yrong commented Sep 3, 2024

alistair-singh commented Sep 3, 2024

yrong Sep 4, 2024 •

edited

Loading

yrong commented Sep 4, 2024

acatangiu commented Sep 4, 2024

yrong commented Sep 4, 2024 •

edited

Loading

yrong commented Sep 4, 2024

acatangiu commented Sep 4, 2024

acatangiu Sep 5, 2024 •

edited

Loading

acatangiu Sep 5, 2024

acatangiu Sep 5, 2024 •

edited

Loading

vgeddes Sep 6, 2024 •

edited

Loading

alistair-singh Sep 6, 2024 •

edited

Loading

vgeddes Sep 6, 2024

vgeddes Sep 6, 2024 •

edited

Loading

vgeddes left a comment

acatangiu commented Sep 6, 2024

	type RegisterTokenOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = AccountIdOf<Self>>;
	type RegisterTokenOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Self::RuntimeOrigin>;

Register origin #174

Are you sure you want to change the base?

Register origin #174

Conversation

yrong commented Sep 3, 2024

alistair-singh commented Sep 3, 2024

yrong Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

yrong commented Sep 4, 2024

acatangiu commented Sep 4, 2024

yrong commented Sep 4, 2024 • edited Loading

yrong commented Sep 4, 2024

acatangiu commented Sep 4, 2024

acatangiu Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

acatangiu Sep 5, 2024

Choose a reason for hiding this comment

acatangiu Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

alistair-singh Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes Sep 6, 2024

Choose a reason for hiding this comment

vgeddes Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes left a comment

Choose a reason for hiding this comment

acatangiu commented Sep 6, 2024

yrong Sep 4, 2024 •

edited

Loading

yrong commented Sep 4, 2024 •

edited

Loading

acatangiu Sep 5, 2024 •

edited

Loading

acatangiu Sep 5, 2024 •

edited

Loading

vgeddes Sep 6, 2024 •

edited

Loading

alistair-singh Sep 6, 2024 •

edited

Loading

vgeddes Sep 6, 2024 •

edited

Loading