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

refactor: remove duplicate chain extensions across runtimes #78

Closed
Tracked by #132
Daanvdplas opened this issue Apr 22, 2024 · 7 comments · Fixed by #163
Closed
Tracked by #132

refactor: remove duplicate chain extensions across runtimes #78

Daanvdplas opened this issue Apr 22, 2024 · 7 comments · Fixed by #163
Assignees
Labels
good first issue Good for newcomers

Comments

@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Apr 22, 2024

I think we should create a separate folder for the extension in the runtime folder, outside devnet/testnet, which the runtimes than take as a dependency, similar to a pallet. We can then have a devnet and testnet feature in the extension crate so that we can deploy new integrations separately first on devnet and then testnet, but still being able to work in one folder instead of all the different runtime folders. The common folder doesn't make sense to me since there are non common things as mentioned above.

Astar has something similar but outside the runtime folder: https://github.com/AstarNetwork/Astar/tree/master/chain-extensions. I would like to maintain it in the runtime folder.

As for the tests, I think we should create a test folder in the chain extension crate where we divide the tests per use case; e.g. fungibles, non-fungiles.

@Daanvdplas Daanvdplas changed the title refactor: replace chainextension tests refactor: replace chainextension + tests May 20, 2024
@al3mart
Copy link
Collaborator

al3mart commented May 20, 2024

Agree on keeping pop exentions in the runtime

@evilrobot-01
Copy link
Collaborator

I guess the rationale for it being outside the runtime directory is simply a classification, where the subdirectories are implementations of runtimes. No big deal either way to me.

I think it probably makes sense, but a few things to consider:

  • features are additive and making them mutually exclusive is possible, but somewhat tricky. The devex may not be great but seperate implementations is also not great. Also consider how it might work with mainnet. Do we end up in feature hell?
  • some type bounds on the extension will probably need improving. For example,
    AccountId = AccountId,
    ensures that an associated type is the same as the AssetId type alias, only defined in that runtime, or possibly common). Unlikely that types will deviate across runtimes, but will moving them result in additional complexity through generics/type bounds, slowing development.

@Daanvdplas
Copy link
Collaborator Author

But will most differences between devnet, testnet and mainnet not be additive? I'm thinking in light of adding new features

I see your point though where constants can be chosen to be different which is not additive, I found this implementation used by frequency which we can take as an inspiration: prod_or_testnet_or_local

@Daanvdplas
Copy link
Collaborator Author

Nevertheless, this requires more careful consideration and refactoring the tests is urgent for now so I will only do that.

@Daanvdplas Daanvdplas changed the title refactor: replace chainextension + tests refactor: remove duplicate chain extensions across runtimes Jul 26, 2024
@Daanvdplas Daanvdplas added the good first issue Good for newcomers label Jul 26, 2024
@chungquantin chungquantin self-assigned this Aug 5, 2024
@chungquantin chungquantin linked a pull request Aug 6, 2024 that will close this issue
@chungquantin
Copy link
Collaborator

I am taking a look at the current testnet extension to migrate, but seems like it is quite out-dated right now (no versioning support so the env buffer is decoded in a different way, first two bytes in devnet is a version while first two bytes in testnet is a function ID), wonder how would we want to handle it?

  1. Should we halt the separation until the pop-api is deployed on testnet with new chain extension and start separating.
  2. Trying to make the separated extension backward compatible with the current testnet chain (but this would not be ideal because testnet is far behind)
  3. The separation PR should apply the devnet chain extension architecture (with versioning) to the testnet as well but disable functionalities related to fungibles.

@evilrobot-01
Copy link
Collaborator

Perhaps it can simply start by moving the extension to common and having devnet use it from there? That should allow easier use by other runtimes when ready.

@chungquantin
Copy link
Collaborator

Close #163

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

Successfully merging a pull request may close this issue.

4 participants