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

[XCM] add generic location to account converter that also works with external ecosystems #7313

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Nathy-bajo
Copy link
Contributor

@Nathy-bajo Nathy-bajo commented Jan 23, 2025

Attempts to resolve #7129 (comment)

I added a new ExternalConsensusLocationsConverterFor struct to handle external global consensus locations and their child locations.
This struct extends the functionality of existing converters (GlobalConsensusParachainConvertsFor and EthereumLocationsConverterFor) while maintaining backward compatibility.

Polkadot address: 121HJWZtD13GJQPD82oEj3gSeHqsRYm1mFgRALu4L96kfPD1

@Nathy-bajo Nathy-bajo requested a review from a team as a code owner January 23, 2025 13:07
@acatangiu acatangiu changed the title [XCM] add generic location to account converter #7129 [XCM] add generic location to account converter that also works with external ecosystems Jan 23, 2025
@Nathy-bajo
Copy link
Contributor Author

Please review @acatangiu

@acatangiu acatangiu marked this pull request as draft January 27, 2025 17:18
@Nathy-bajo Nathy-bajo marked this pull request as ready for review February 11, 2025 13:00
@Nathy-bajo
Copy link
Contributor Author

Nathy-bajo commented Feb 11, 2025

@acatangiu I'm still a bit confused but I tried following your explanation.

@Nathy-bajo Nathy-bajo requested a review from acatangiu February 12, 2025 10:25
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

yes, you are on the right track

@Nathy-bajo Nathy-bajo requested a review from acatangiu February 19, 2025 15:12
@Nathy-bajo
Copy link
Contributor Author

Can you help review this please @acatangiu

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Looks good! Now you need to add tests.

You can extend this test with more conversions.

Please also add explicit checks to the test that verify the new ExternalConsensusLocationsConverterFor converts locations previously covered by GlobalConsensusParachainConvertsFor and EthereumLocationsConverterFor with the same results.

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.

[XCM] add generic location to account converter for locations in external ecosystems
2 participants