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

kusama: General Admin to send Location mapping #383

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Jul 15, 2024

Fixes: #382

General Admin Origin to xcm Location mapping for outgoing messages

@bkchr
Copy link
Contributor

bkchr commented Jul 15, 2024

/merge

@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) July 15, 2024 22:40
auto-merge was automatically disabled July 16, 2024 09:40

Head branch was pushed to by a user without write access

@muharem
Copy link
Contributor Author

muharem commented Jul 16, 2024

@bkchr @acatangiu FYI, I have added the tests and updated the changelog

use kusama_runtime::governance::pallet_custom_origins::Origin::GeneralAdmin as GeneralAdminOrigin;

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

Choose a reason for hiding this comment

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

nit:

Deduplicate general_admin_add_registrar and relay_root_add_registrar tests code since they do the same thing with same result, just using different origins.

Keep both tests, that just call helper fn with different origins.

Copy link
Contributor Author

@muharem muharem Jul 16, 2024

Choose a reason for hiding this comment

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

I left one test function relay_commands_add_registrar with a loop over two origins.

I personally do not like function abstraction when there is no clear (semantic) abstraction. This is often the case for tests. Frequently, we just move duplicated code into a function with a long name, which makes tests harder to read and maintain. I prefer copy-pasting over poor abstraction. Additionally, such abstractions introduce a variety of different styles to the codebase. I also prefer consistency in code style. A test framework is usually helpful in maintaining this consistency. For example, in my case, a data provider for a test function could be used.

/// Type to convert a pallet `Origin` type value into a `Location` value which represents an
/// interior location of this chain for a destination chain.
pub type LocalPalletOrSignedOriginToLocation = (
// GeneralAdmin origin to be used in XCM as a corresponding Plurality `Location` value.
GeneralAdminToPlurality,
Copy link
Contributor

Choose a reason for hiding this comment

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

orthogonal to this PR, but quick Q: Kusama is also missing TreasurerToPlurality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see a use case for it now. In Polkadot we need it to manage Fellowship Treasury on Collectives.

@muharem
Copy link
Contributor Author

muharem commented Jul 17, 2024

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 7a5c66f into polkadot-fellows:main Jul 17, 2024
44 checks passed
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[People] Add new registrar
3 participants