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

Move chain specific definitions #440

Merged

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Mar 25, 2024

Closes #383.

Definitions used in only one chain are moved to the new definitions.rs file for each runtime.
Also removes previously hardcoded multi-locations which now are taken from the asset registry.

Additionally, we remove the common definition of chain extensions which is not needed after #437.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
@gianfra-t gianfra-t linked an issue Mar 25, 2024 that may be closed by this pull request
@gianfra-t gianfra-t changed the title moved chain specific definitions Move chain specific definitions Mar 25, 2024
@gianfra-t gianfra-t marked this pull request as ready for review March 25, 2024 20:09
@gianfra-t gianfra-t requested a review from a team March 25, 2024 20:09
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

I generally like it 👍 we only have to split the asset hub stuff.

Though I guess that most of the definitions will actually get obsolete once #432 is merged, right? Because we don't necessarily need to keep them around.

runtime/common/src/lib.rs Outdated Show resolved Hide resolved
@gianfra-t
Copy link
Contributor Author

gianfra-t commented Mar 27, 2024

I guess that most of the definitions will actually get obsolete once #432 is merged

Yes... I guess it is better to wait for that PR to be merged and check what we actually need, so we avoid cleaning twice.

@TorstenStueber
Copy link
Contributor

@gianfra-t would it make change the status of this PR to "Draft" for this reason?

runtime/common/src/lib.rs Outdated Show resolved Hide resolved
runtime/amplitude/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/sibling.rs Outdated Show resolved Hide resolved
@gianfra-t gianfra-t marked this pull request as draft April 2, 2024 13:02
@gianfra-t
Copy link
Contributor Author

After the upgrade from #432 to use the asset registry, I believe we will no longer need many "hardcoded" locations defined before (for instance, these)

@gianfra-t gianfra-t marked this pull request as ready for review April 9, 2024 13:40
@bogdanS98 bogdanS98 self-requested a review April 9, 2024 13:53
Copy link
Contributor

@bogdanS98 bogdanS98 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 to me 👍🏼 Though we should move PINK definitions to Pendulum.

runtime/amplitude/src/definitions.rs Outdated Show resolved Hide resolved
runtime/amplitude/src/definitions.rs Outdated Show resolved Hide resolved
@gianfra-t
Copy link
Contributor Author

@pendulum-chain/devs I have removed even more definitions that I believe we only used for currency conversion in the past. See for instance, here. The runtime compiles but I am afraid I am missing some use case for this constants, so please check.

I decided to leave definitions.rs even though is empty for some chains in case we want to add some in the future.

Copy link
Contributor

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

I am not sure whether it makes sense to have an assets.rs and a definition.rs file per runtime. At the end the definitions inside definition.rs are all asset related and for that reason it would make sense to just join both files.

On the other hand: are the remaining definitions in assets.rs still needed? At the end we don't want to have a need to execute runtime upgrades when we register new XCM assets – that's why we moved all the logic into the asset registry pallet. But if we register new assets without changing the code, then the definitions in asset.rs deviate from the truth.

Looks to me like these definitions are only needed in the integration tests, correct? Is there a way to load the information from the asset-registry storage inside the tests instead and then we can remove the remaining content of all asset.rs files.

runtime/foucoco/src/assets.rs Outdated Show resolved Hide resolved
runtime/common/src/chain_ext.rs Show resolved Hide resolved
@gianfra-t
Copy link
Contributor Author

@TorstenStueber there are very few definitions from the assets files actually used, which I now moved to the pendulum definitions file. You are correct that they are only used in our integration tests (except for BRZ which is also used for the automation routing for the BRZ token).

We use these definitions here for defining the currency conversion of the sibling chain but also in our testing configuration of the asset registry for pendulum and amplitude. For this reason I think these definitions are useful even when we use the asset registry because it saves us having to manually define the location.

Alternatively they could be moved to the integration test directory (except for BRZ).

@TorstenStueber
Copy link
Contributor

I prefer to move definitions that are only used for testing into testing related code in order to not "clutter up" the rest. What do you think?

@gianfra-t
Copy link
Contributor Author

Yes! It makes sense. Moved to a definitions.rs in the integration-tests folder.

Copy link
Contributor

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@gianfra-t gianfra-t requested a review from bogdanS98 April 11, 2024 13:08
Copy link
Contributor

@bogdanS98 bogdanS98 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 👍🏼

@gianfra-t gianfra-t merged commit a6b2e0f into main Apr 19, 2024
2 checks passed
@gianfra-t gianfra-t deleted the 383-move-parachain-specific-definitions-out-of-runtime-common branch April 19, 2024 18:36
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.

Move parachain specific definitions out of runtime-common
5 participants