search

pendulum-chain / pendulum

GNU General Public License v3.0
43 stars 14 forks source link

Move chain specific definitions #440

Closed gianfra-t closed 2 months ago

gianfra-t commented 3 months ago

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.

gianfra-t commented 3 months ago

I guess that most of the definitions will actually get obsolete once https://github.com/pendulum-chain/pendulum/pull/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 commented 3 months ago

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

gianfra-t commented 2 months ago

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 commented 2 months ago

@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.

gianfra-t commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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