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

split template into 2 opinionated runtimes: AccountId20 + AccountId32 #1061

Closed
wants to merge 12 commits into from

Conversation

bernardoaraujor
Copy link
Contributor

close #1056

@bernardoaraujor bernardoaraujor requested a review from sorpaas as a code owner May 23, 2023 22:49
@bernardoaraujor bernardoaraujor force-pushed the bar/split-template-runtimes branch from 8dccb25 to f7d72b0 Compare May 25, 2023 22:21
@sorpaas
Copy link
Member

sorpaas commented Jun 23, 2023

My problem with this approach is that we now have a lot of duplicate code. You may think we can work with them alright, but it actually is a major source of pain whenever the runtime needs changes (which is frequent).

Would you mind to implement everything as feature gate instead? We continue to have a single runtime but just optionally built with either account types. I saw you already did it like that for some of the types and client construction code.

Also the feature gate accountid32/accountid20 is redundant. You just need to have one feature accountid20. If it's not enabled then we use accountid32.

@bernardoaraujor
Copy link
Contributor Author

My problem with this approach is that we now have a lot of duplicate code. You may think we can work with them alright, but it actually is a major source of pain whenever the runtime needs changes (which is frequent).

Would you mind to implement everything as feature gate instead? We continue to have a single runtime but just optionally built with either account types. I saw you already did it like that for some of the types and client construction code.

Also the feature gate accountid32/accountid20 is redundant. You just need to have one feature accountid20. If it's not enabled then we use accountid32.

closing this PR in favor of #1186

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

Successfully merging this pull request may close these issues.

split template into 2 opinionated runtimes: AccountId20 + AccountId32
3 participants