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

Drop Tranche location #1336

Closed
NunoAlexandre opened this issue May 9, 2023 · 5 comments · Fixed by #1340
Closed

Drop Tranche location #1336

NunoAlexandre opened this issue May 9, 2023 · 5 comments · Fixed by #1340
Assignees
Labels
I6-refactoring Code needs refactoring. P5-soon Issue should be addressed soon. Q0-trivial An issure which is similar to patching code.

Comments

@NunoAlexandre
Copy link
Contributor

NunoAlexandre commented May 9, 2023

Currently, the pool_system::tranches::create_metadata function defines the AssetMetadata.location field as follows:

location: Some(VersionedMultiLocation::V3(MultiLocation {
	parents: 1,
	interior: X3(
		Parachain(parachain_id.into()),
		PalletInstance(pallet_index),
		// todo(nuno): revisit this
		GeneralKey {
			length: 16u8,
			data: vec_to_fixed_array(tranche_id),
		},
	),
})),

I remember suggesting this structure many months back, but as I am updating the codebase to Polkadot v0.9.38 and XCM v3 and bumped into this, I realised we don't need a location for tranche tokens at all. Tranche tokens are not to be handled through XCM due to their permissions nature and we are building support for permissioned transfers through Connectors.

Suggestion

Basically set location to None and adjust all tests.

+ location: None,
- location: Some(VersionedMultiLocation::V3(MultiLocation {
- 	parents: 1,
- 	interior: X3(
- 		Parachain(parachain_id.into()),
- 		PalletInstance(pallet_index),
- 		// todo(nuno): revisit this
- 		GeneralKey {
- 			length: 16u8,
- 			data: vec_to_fixed_array(tranche_id),
- 		},
- 	),
- })),
@NunoAlexandre NunoAlexandre self-assigned this May 9, 2023
@NunoAlexandre
Copy link
Contributor Author

@mustermeiszer @offerijns what do you think? Any objections?

@mustermeiszer mustermeiszer added Q0-trivial An issure which is similar to patching code. P5-soon Issue should be addressed soon. I6-refactoring Code needs refactoring. labels May 10, 2023
@mustermeiszer
Copy link
Collaborator

Sounds good and agreed!

@NunoAlexandre
Copy link
Contributor Author

Great, done in 61f2960. I will also include a migration that sets the location of any existing tranche token in the registry to None.

@NunoAlexandre NunoAlexandre mentioned this issue May 10, 2023
2 tasks
@mustermeiszer
Copy link
Collaborator

I am not the biggest fan of having this in 38. But I also know that having intermediate branches is annoying. But could cherru pick the commit and create a PR. Especially given we need a migration. WDYT?

@NunoAlexandre
Copy link
Contributor Author

I am not the biggest fan of having this in 38. But I also know that having intermediate branches is annoying. But could cherru pick the commit and create a PR. Especially given we need a migration. WDYT?

You are absolutely right. I am always advocating for smaller, self-contained PRs but this time I went against my own principle. I will backport it to a new PR based of the latest main and add the migration part as well. Thanks for the feedback 🤟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I6-refactoring Code needs refactoring. P5-soon Issue should be addressed soon. Q0-trivial An issure which is similar to patching code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants